feat: added initial live DAST server implementation #5772
Conversation
… nuclei-dast-server
…scovery/nuclei into nuclei-dast-server
WalkthroughThe changes in this pull request introduce significant enhancements to the nuclei application, particularly in the area of Dynamic Application Security Testing (DAST). Key modifications include the addition of new command-line flags for configuring DAST server options, the introduction of a name generator, and improvements to the handling of fuzzing options. New files have been added to manage HTTP request deduplication, scope management, and fuzzing statistics using a SQLite database. Additionally, various dependencies have been updated or added to support these features. Changes
Poem
🪧 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: 32
🧹 Outside diff range and nitpick comments (55)
pkg/input/formats/yaml/multidoc_test.go (1)
21-24: LGTM with a minor suggestion for error message.The file handling implementation follows Go best practices with proper error checking and resource cleanup.
Consider simplifying the error message format:
-require.Nilf(t, err, "error opening proxify input file: %v", err) +require.Nilf(t, err, "failed to open proxify input file: %v", err)pkg/input/formats/burp/burp_test.go (1)
22-25: Consider enhancing error handling after Parse.While the changes look good, consider using
require.NoErrorinstead of the current if-fatal pattern for consistency with the error handling style used above.- err = format.Parse(file, func(request *types.RequestResponse) bool { - gotMethodsToURLs = append(gotMethodsToURLs, request.URL.String()) - return false - }, proxifyInputFile) - if err != nil { - t.Fatal(err) - } + err = format.Parse(file, func(request *types.RequestResponse) bool { + gotMethodsToURLs = append(gotMethodsToURLs, request.URL.String()) + return false + }, proxifyInputFile) + require.NoError(t, err, "failed to parse burp file")internal/server/scope/scope_test.go (1)
10-11: Consider enhancing test structure and naming.While the test follows Go testing best practices, consider making the test cases more explicit:
- Rename the subtest from "url" to something more specific like "should_allow_matching_paths_and_reject_excluded_patterns"
- Consider adding more test cases for edge cases
func TestManagerValidate(t *testing.T) { - t.Run("url", func(t *testing.T) { + t.Run("should_allow_matching_paths_and_reject_excluded_patterns", func(t *testing.T) {pkg/input/formats/swagger/swagger_test.go (1)
18-20: LGTM! Consider simplifying the error message.The file handling implementation follows Go best practices with proper resource management and error handling.
Consider simplifying the error message as the error is already included:
-require.Nilf(t, err, "error opening proxify input file: %v", err) +require.Nilf(t, err, "failed to open proxify input file")pkg/authprovider/authx/strategy.go (1)
35-37: Ensure consistent behavior with Apply methodThe
ApplyOnRRimplementation mirrorsApplybut has the same potential issues. Additionally, since these methods share similar logic, consider extracting the common pattern.Consider creating a helper method to reduce code duplication:
+func (d *DynamicAuthStrategy) applyStrategies(apply func(AuthStrategy)) { + strategies := d.Dynamic.GetStrategies() + if strategies == nil { + return + } + for _, s := range strategies { + if s == nil { + continue + } + apply(s) + } +} func (d *DynamicAuthStrategy) Apply(req *http.Request) { - strategy := d.Dynamic.GetStrategies() - for _, s := range strategy { - s.Apply(req) - } + d.applyStrategies(func(s AuthStrategy) { + s.Apply(req) + }) } func (d *DynamicAuthStrategy) ApplyOnRR(req *retryablehttp.Request) { - strategy := d.Dynamic.GetStrategies() - for _, s := range strategy { - s.ApplyOnRR(req) - } + d.applyStrategies(func(s AuthStrategy) { + s.ApplyOnRR(req) + }) }pkg/input/formats/openapi/openapi.go (1)
Line range hint
36-40: Consider enhancing error handling for io.Reader specific failuresWhile the implementation correctly uses
LoadFromIoReader, consider adding specific error handling for io.Reader failures (e.g., network timeouts, connection issues) to provide better error context in a live DAST environment.func (j *OpenAPIFormat) Parse(input io.Reader, resultsCb formats.ParseReqRespCallback, filePath string) error { loader := openapi3.NewLoader() - schema, err := loader.LoadFromIoReader(input) + data, err := io.ReadAll(input) + if err != nil { + return errors.Wrap(err, "could not read openapi schema") + } + schema, err := loader.LoadFromData(data) if err != nil { return errors.Wrap(err, "could not decode openapi 3.0 schema") }pkg/fuzz/stats/db_test.go (2)
13-22: Enhance test coverage for inserted fieldsWhile the test verifies the insertion succeeds, it doesn't validate that all fields were correctly stored in the database.
Consider adding verification for all fields:
require.NoError(t, err) + + // Verify all fields were stored correctly + var event FuzzingEvent + err = db.db.QueryRow(` + SELECT url, site_name, template_id, component_type, + component_name, payload_sent, status_code + FROM components WHERE id = 1 + `).Scan(&event.URL, &event.SiteName, &event.TemplateID, + &event.ComponentType, &event.ComponentName, + &event.PayloadSent, &event.StatusCode) + require.NoError(t, err) + require.Equal(t, "http://localhost:8080/login", event.URL) + require.Equal(t, "localhost:8080", event.SiteName) + require.Equal(t, "apache-struts2-001", event.TemplateID) + require.Equal(t, "path", event.ComponentType) + require.Equal(t, "/login", event.ComponentName) + require.Equal(t, "/login'\"><", event.PayloadSent) + require.Equal(t, 401, event.StatusCode)
29-39: Refactor duplicate test data and verify matched statusThe test has two issues:
- Test data is duplicated from the first insertion
- The matched status isn't verified
Consider these improvements:
+ // Define test data once + testEvent := FuzzingEvent{ + URL: "http://localhost:8080/login", + SiteName: "localhost:8080", + TemplateID: "apache-struts2-001", + ComponentType: "path", + ComponentName: "/login", + PayloadSent: "/login'\"><", + StatusCode: 401, + } + + // Test unmatched insertion + err = db.InsertComponent(testEvent) + require.NoError(t, err) + + // Test matched insertion + testEvent.Matched = true + err = db.InsertMatchedRecord(testEvent) + require.NoError(t, err) + + // Verify matched status + var matched bool + err = db.db.QueryRow("SELECT matched FROM matched_records WHERE id = 1").Scan(&matched) + require.NoError(t, err) + require.True(t, matched)internal/server/scope/extensions.go (1)
13-33: Improve maintainability with structured extension groups.The extension map would be more maintainable if organized using constants or variables for each category.
Consider restructuring like this:
+// Common file extensions that are typically not interesting for security scanning +var ( + imageExtensions = map[string]struct{}{ + ".jpg": {}, ".jpeg": {}, ".png": {}, ".gif": {}, ".bmp": {}, ".tiff": {}, ".ico": {}, + } + mediaExtensions = map[string]struct{}{ + ".mp4": {}, ".avi": {}, ".mov": {}, ".wmv": {}, ".flv": {}, ".mkv": {}, ".webm": {}, + } + // ... other categories +) + +// excludedExtensions combines all extension categories +var excludedExtensions = make(map[string]struct{}) + +func init() { + // Populate excludedExtensions from categories + for ext := range imageExtensions { + excludedExtensions[ext] = struct{}{} + } + // ... add other categories +}pkg/input/formats/burp/burp.go (3)
39-42: Enhance error context with file informationWhen wrapping the error, consider including the format name to provide better context for debugging.
items, err := burpxml.Parse(input, true) if err != nil { - return errors.Wrap(err, "could not decode burp xml schema") + return errors.Wrap(err, "burp: could not decode xml schema") }
Line range hint
56-56: Address the TODO comment regarding callback handlingThe comment indicates that the callback's return value should be handled, but it's currently being ignored. This could lead to issues if the callback is designed to signal processing errors or early termination.
Here's a suggested implementation:
- resultsCb(rawRequest) // TODO: Handle false and true from callback + if !resultsCb(rawRequest) { + // Stop processing if callback returns false + return nil + }Would you like me to help implement proper callback handling or create an issue to track this?
Line range hint
44-56: Consider adding error context for base64 decoding failuresWhen base64 decoding fails, it would be helpful to know which item in the burp file caused the failure.
binx, err := base64.StdEncoding.DecodeString(item.Request.Raw) if err != nil { - return errors.Wrap(err, "could not decode base64") + return errors.Wrapf(err, "could not decode base64 for request at URL: %s", item.Url) }pkg/input/formats/openapi/openapi_test.go (1)
45-48: Simplify error message in require.NilfThe error message includes the error value twice: once in the format string and once as an argument. Consider simplifying it.
-require.Nilf(t, err, "error opening proxify input file: %v", err) +require.Nilf(t, err, "error opening proxify input file")pkg/input/formats/json/json.go (1)
48-48: Consider documenting or removing the unusedfilePathparameter.The
filePathparameter is currently unused in the implementation. Either:
- Document its intended future use with a TODO comment, or
- Remove it if it's not needed
-func (j *JSONFormat) Parse(input io.Reader, resultsCb formats.ParseReqRespCallback, filePath string) error { +func (j *JSONFormat) Parse(input io.Reader, resultsCb formats.ParseReqRespCallback) error {pkg/input/formats/swagger/swagger.go (1)
43-44: Add validation for empty filePath parameterThe extension checking logic assumes filePath will always have an extension. Consider adding validation to handle cases where filePath might be empty or without an extension.
+if filePath == "" { + return errors.New("filePath parameter is required") +} ext := path.Ext(filePath) +if ext == "" { + return errors.New("could not determine file format: missing file extension") +}Also applies to: 46-47
pkg/input/formats/yaml/multidoc.go (2)
48-48: Remove unused parameterThe
filePathparameter is not used in the implementation. Consider either:
- Removing it if not needed
- Adding a comment explaining why it's required for interface compatibility
Line range hint
49-61: Fix incorrect error message in YAML parserThe error message mentions "json file" but this is a YAML parser.
Apply this fix:
- return errors.Wrap(err, "could not decode json file") + return errors.Wrap(err, "could not decode yaml file")pkg/input/formats/json/json_test.go (1)
50-53: Consider standardizing error handling approachFor consistency with the error handling style used above, consider replacing the manual error check with require.NoError.
- if err != nil { - t.Fatal(err) - } + require.NoError(t, err)Also applies to: 55-57
internal/server/scope/scope.go (4)
1-8: Improve attribution commentThe comment "From Katana" should be more specific, including the source repository URL and license information if applicable.
-// From Katana +// Adapted from Katana (https://github.com/projectdiscovery/katana) +// Licensed under MIT License
10-15: Enhance type documentationWhile the struct is well-designed, the documentation could be more detailed to explain the purpose of each field.
-// Manager manages scope for crawling process +// Manager manages URL scope validation for DAST crawling process. +// It maintains lists of regular expressions for in-scope and out-of-scope URLs, +// with a flag to indicate if scope checking is disabled. type Manager struct { + // inScope contains compiled regex patterns for URLs that should be included inScope []*regexp.Regexp + // outOfScope contains compiled regex patterns for URLs that should be excluded outOfScope []*regexp.Regexp + // noScope indicates whether scope checking is disabled (true when no patterns are defined) noScope bool }
17-39: Refactor duplicate regex compilation logicThe regex compilation logic is duplicated for inScope and outOfScope. Consider extracting this into a helper function.
+func compilePatterns(patterns []string) ([]*regexp.Regexp, error) { + var compiled []*regexp.Regexp + for _, regex := range patterns { + pattern, err := regexp.Compile(regex) + if err != nil { + return nil, fmt.Errorf("could not compile regex %s: %s", regex, err) + } + compiled = append(compiled, pattern) + } + return compiled, nil +} func NewManager(inScope, outOfScope []string) (*Manager, error) { manager := &Manager{} - for _, regex := range inScope { - if compiled, err := regexp.Compile(regex); err != nil { - return nil, fmt.Errorf("could not compile regex %s: %s", regex, err) - } else { - manager.inScope = append(manager.inScope, compiled) - } + var err error + manager.inScope, err = compilePatterns(inScope) + if err != nil { + return nil, err } - for _, regex := range outOfScope { - if compiled, err := regexp.Compile(regex); err != nil { - return nil, fmt.Errorf("could not compile regex %s: %s", regex, err) - } else { - manager.outOfScope = append(manager.outOfScope, compiled) - } + + manager.outOfScope, err = compilePatterns(outOfScope) + if err != nil { + return nil, err }
59-77: Optimize validation logic with early returnsThe validation logic can be made more efficient with early returns and avoiding unnecessary variable declaration.
func (m *Manager) validateURL(URL string) (bool, error) { for _, item := range m.outOfScope { if item.MatchString(URL) { return false, nil } } if len(m.inScope) == 0 { return true, nil } - var inScopeMatched bool for _, item := range m.inScope { if item.MatchString(URL) { - inScopeMatched = true - break + return true, nil } } - return inScopeMatched, nil + return false, nil }pkg/fuzz/stats/stats.go (3)
41-53: Add field documentation to FuzzingEvent structConsider adding documentation for each field to improve code maintainability.
type FuzzingEvent struct { + // URL is the target URL being fuzzed URL string + // ComponentType represents the type of component being tested ComponentType string + // ComponentName is the identifier of the component ComponentName string // ... (continue for other fields) }
65-81: Enhance URL parsing robustnessThe current implementation has a few areas for improvement:
- Silent error handling might hide issues
- No input validation for empty/malformed URLs
Consider this enhanced implementation:
-func getCorrectSiteName(originalURL string) string { +func getCorrectSiteName(originalURL string) (string, error) { + if originalURL == "" { + return "", fmt.Errorf("empty URL provided") + } + parsed, err := url.Parse(originalURL) if err != nil { - return "" + return "", fmt.Errorf("failed to parse URL: %w", err) } // Site is the host:port combo siteName := parsed.Host + if siteName == "" { + return "", fmt.Errorf("no host found in URL") + } + if parsed.Port() == "" { if parsed.Scheme == "https" { siteName = fmt.Sprintf("%s:443", siteName) } else if parsed.Scheme == "http" { siteName = fmt.Sprintf("%s:80", siteName) } } - return siteName + return siteName, nil }
1-81: Consider adding metrics and monitoring capabilitiesWhile the current implementation provides basic statistics tracking, consider enhancing it with:
- Metrics collection for monitoring fuzzing performance
- Rate limiting or throttling mechanisms
- Periodic statistics aggregation for long-running fuzzing sessions
These additions would make the system more observable and maintainable in production environments.
🧰 Tools
🪛 GitHub Check: Lint
[failure] 57-57:
Error return value oft.database.InsertMatchedRecordis not checked (errcheck)
[failure] 62-62:
Error return value oft.database.InsertComponentis not checked (errcheck)pkg/fuzz/component/cookie.go (1)
Line range hint
86-95: Potential improvement: Consider cookie attributesThe cookie rebuilding logic only preserves the Name and Value attributes. Consider preserving other security-relevant attributes like Secure, HttpOnly, SameSite, etc.
cookie := &http.Cookie{ Name: key, Value: fmt.Sprint(value), + Secure: true, // If the original cookie was secure + HttpOnly: true, // If the original cookie was HttpOnly + SameSite: http.SameSiteStrictMode, // Based on original cookie }pkg/fuzz/stats/schema.sql (5)
1-5: Consider adding audit fields to the sites tableFor better data management and auditing capabilities, consider adding the following fields:
created_atDATETIME DEFAULT CURRENT_TIMESTAMPupdated_atDATETIMEis_deletedBOOLEAN DEFAULT FALSECREATE TABLE IF NOT EXISTS sites ( site_id INTEGER PRIMARY KEY AUTOINCREMENT, site_name TEXT UNIQUE NOT NULL, + created_at DATETIME DEFAULT CURRENT_TIMESTAMP, + updated_at DATETIME, + is_deleted BOOLEAN DEFAULT FALSE );
21-25: Add template metadata fieldsConsider adding fields to track template metadata and lifecycle:
- Audit timestamps
- Template version/hash for tracking changes
- Template status (active/deprecated)
CREATE TABLE IF NOT EXISTS templates ( template_id INTEGER PRIMARY KEY AUTOINCREMENT, - template_name TEXT UNIQUE NOT NULL + template_name TEXT UNIQUE NOT NULL, + template_version TEXT, + template_hash TEXT, + is_active BOOLEAN DEFAULT TRUE, + created_at DATETIME DEFAULT CURRENT_TIMESTAMP, + updated_at DATETIME );
51-64: Add error handling to the triggerConsider adding checks to handle edge cases:
- Validate that the component exists before updating
- Add error handling for the case where template_id doesn't exist
CREATE TRIGGER IF NOT EXISTS update_component_stats AFTER INSERT ON fuzzing_results BEGIN + SELECT RAISE(ROLLBACK, 'Component not found') + WHERE NOT EXISTS ( + SELECT 1 FROM components WHERE component_id = NEW.component_id + ); + + SELECT RAISE(ROLLBACK, 'Template not found') + WHERE NOT EXISTS ( + SELECT 1 FROM templates WHERE template_id = NEW.template_id + ); UPDATE components SET last_fuzzed = NEW.timestamp, total_fuzz_count = total_fuzz_count + 1 WHERE component_id = NEW.component_id; INSERT INTO component_templates (component_id, template_id, times_applied) VALUES (NEW.component_id, NEW.template_id, 1) ON CONFLICT(component_id, template_id) DO UPDATE SET times_applied = times_applied + 1; END;
67-71: Implement data management strategiesConsider the following improvements for the
fuzzing_request_responsetable:
- Add size limits or use BLOB with compression for raw_request and raw_response
- Implement a retention policy for old data
- Add timestamp for data lifecycle management
CREATE TABLE IF NOT EXISTS fuzzing_request_response ( request_id INTEGER PRIMARY KEY AUTOINCREMENT, - raw_request TEXT NOT NULL, - raw_response TEXT NOT NULL + raw_request BLOB NOT NULL, + raw_response BLOB NOT NULL, + created_at DATETIME DEFAULT CURRENT_TIMESTAMP ); +-- Consider adding a periodic cleanup job or implementing partitioning +-- CREATE INDEX idx_fuzzing_request_response_created_at ON fuzzing_request_response(created_at);
1-71: Add database configuration and documentationConsider adding:
- Database configuration pragmas for SQLite optimization
- Schema version tracking
- Documentation for each table's purpose
Add at the beginning of the file:
-- Schema version: 1.0.0 PRAGMA foreign_keys = ON; PRAGMA journal_mode = WAL; PRAGMA synchronous = NORMAL; -- Table documentation -- sites: Stores target sites for fuzzing -- components: Stores components (paths, headers, etc.) identified for fuzzing -- templates: Stores fuzzing templates -- component_templates: Tracks template execution against components -- fuzzing_results: Stores results of fuzzing operations -- fuzzing_request_response: Stores raw HTTP request/response datapkg/input/formats/formats.go (1)
39-39: Consider enhancing error context for DAST server use caseSince this is part of a DAST server implementation, consider enriching error returns with additional context (like file position for parsing errors) to help with debugging and error reporting in the server environment.
Example enhancement:
// Consider defining a richer error type type ParseError struct { FilePath string Position int64 Err error } func (e *ParseError) Error() string { return fmt.Sprintf("parse error in %s at position %d: %v", e.FilePath, e.Position, e.Err) }internal/runner/lazy.go (1)
Line range hint
138-146: Enhance error handling specificityThe current error handling uses generic errors with string messages. Consider creating specific error types for different scenarios (no match, no values, extraction failure) to enable better error handling by callers.
Consider creating specific error types:
+type ExtractionError struct { + Template string + Reason string +} + +func (e *ExtractionError) Error() string { + return fmt.Sprintf("%s: %s", e.Template, e.Reason) +} if len(data) == 0 { if e.OperatorsResult.Matched { - finalErr = fmt.Errorf("match found but no (dynamic/extracted) values found for template: %s", d.TemplatePath) + finalErr = &ExtractionError{ + Template: d.TemplatePath, + Reason: "match found but no dynamic or extracted values", + } } else { - finalErr = fmt.Errorf("no match or (dynamic/extracted) values found for template: %s", d.TemplatePath) + finalErr = &ExtractionError{ + Template: d.TemplatePath, + Reason: "no match or values found", + } } }pkg/authprovider/file.go (2)
Line range hint
90-105: Consider memory management for compiled regex patternsThe compiled regex patterns are stored in the
f.compiledmap without any cleanup mechanism. For long-running DAST servers, this could lead to memory growth if many dynamic secrets are processed over time.Consider implementing one of these solutions:
- Add a cleanup mechanism for unused patterns
- Implement a cache with expiration for compiled patterns
- Move regex compilation to a separate goroutine with proper lifecycle management
Line range hint
106-120: Enhance domain normalization for comprehensive coverageThe current domain normalization is limited and might miss edge cases. This could lead to authentication bypass in certain scenarios.
Consider these improvements:
domain = strings.TrimSpace(domain) +domain = strings.ToLower(domain) // Normalize case +if strings.HasPrefix(domain, "[") && strings.HasSuffix(domain, "]") { + // Handle IPv6 addresses + domain = strings.TrimPrefix(domain, "[") + domain = strings.TrimSuffix(domain, "]") +} domain = strings.TrimSuffix(domain, ":80") domain = strings.TrimSuffix(domain, ":443") +// Handle IDN domains +if punycodeHost, err := idna.ToASCII(domain); err == nil { + domain = punycodeHost +}Additionally, consider using a proper URL parsing library to handle all edge cases consistently.
pkg/protocols/protocols.go (1)
103-104: Consider adding documentation for the new field.The field addition is well-placed near other fuzzing-related fields. Consider adding a more detailed comment to document:
- The purpose of this tracker
- Whether it's optional or required for DAST operations
- Any initialization requirements
Example documentation:
+ // FuzzStatsDB tracks fuzzing statistics in a SQLite database for DAST operations. + // It records component events and results during fuzzing operations. FuzzStatsDB *stats.Trackerpkg/catalog/loader/loader.go (1)
512-513: Consider extracting the complex condition into a methodThe condition combines multiple checks which could be made more readable by extracting it into a dedicated method.
Consider refactoring to:
+func (t *Template) HasGlobalMatchers() bool { + return t.Options != nil && t.Options.GlobalMatchers != nil && t.Options.GlobalMatchers.HasMatchers() +} -if parsed.IsFuzzing() || parsed.Options.GlobalMatchers != nil && parsed.Options.GlobalMatchers.HasMatchers() { +if parsed.IsFuzzing() || parsed.HasGlobalMatchers() {pkg/types/types.go (1)
416-423: Consider adding validation and defaults for DAST server configuration.While the DAST server fields are well-documented, consider the following improvements:
- Add validation for
DASTServerAddressto ensure it's a valid URL/address format- Implement a default value mechanism for
DASTScanNameto auto-generate unique scan names// DASTServerAddress is the address for the dast server DASTServerAddress string +// DefaultDASTServerAddress is the default address for the dast server +DefaultDASTServerAddress string = "localhost:8080"pkg/protocols/http/request.go (2)
933-943: LGTM! Consider adding error handling for database operations.The implementation for recording fuzzing component events is well-structured and captures all relevant metadata. However, since this involves database operations, it would be good to handle potential errors.
Consider adding error handling:
if request.options.FuzzStatsDB != nil && generatedRequest.fuzzGeneratedRequest.Request != nil { - request.options.FuzzStatsDB.RecordComponentEvent(fuzzStats.FuzzingEvent{ + if err := request.options.FuzzStatsDB.RecordComponentEvent(fuzzStats.FuzzingEvent{ URL: input.MetaInput.Target(), SiteName: hostname, TemplateID: request.options.TemplateID, ComponentType: generatedRequest.fuzzGeneratedRequest.Component.Name(), ComponentName: generatedRequest.fuzzGeneratedRequest.Parameter, PayloadSent: generatedRequest.fuzzGeneratedRequest.Value, StatusCode: respChain.Response().StatusCode, - }) + }); err != nil { + gologger.Warning().Msgf("Could not record fuzzing component event: %v", err) + } }
1038-1051: LGTM! Be mindful of memory usage with raw request/response storage.The implementation for recording fuzzing results is correct and comprehensive. However, storing raw request and response data could consume significant memory/storage space over time.
Consider implementing one or more of these optimizations:
- Add configurable size limits for raw data storage
- Implement data rotation/cleanup policies
- Use compression for raw data storage
- Consider storing large raw data in a separate table/collection with TTL
internal/server/requests_worker.go (1)
14-14: Remove redundant newline characters from log messagesThe
gologgermethods automatically append newlines to log messages. Including\nin the messages can lead to extra blank lines in the output.Remove the
\nfromMsgflog messages:-gologger.Warning().Msgf("Could not parse raw request: %s\n", err) +gologger.Warning().Msgf("Could not parse raw request: %s", err) ... -gologger.Warning().Msgf("Uninteresting path: %s\n", parsedReq.URL.Path) +gologger.Warning().Msgf("Uninteresting path: %s", parsedReq.URL.Path) ... -gologger.Warning().Msgf("Could not validate scope: %s\n", err) +gologger.Warning().Msgf("Could not validate scope: %s", err) ... -gologger.Warning().Msgf("Request is out of scope: %s %s\n", parsedReq.Request.Method, parsedReq.URL.String()) +gologger.Warning().Msgf("Request is out of scope: %s %s", parsedReq.Request.Method, parsedReq.URL.String()) ... -gologger.Warning().Msgf("Duplicate request detected: %s %s\n", parsedReq.Request.Method, parsedReq.URL.String()) +gologger.Warning().Msgf("Duplicate request detected: %s %s", parsedReq.Request.Method, parsedReq.URL.String()) ... -gologger.Warning().Msgf("Could not run nuclei: %s\n", err) +gologger.Warning().Msgf("Could not run nuclei: %s", err)Also applies to: 25-25, 31-31, 35-35, 40-40, 53-53
internal/server/dedupe.go (2)
15-32: Review the selection of dynamic headers for completeness.The
dynamicHeadersmap includes headers that are typically dynamic and should be excluded from deduplication. However, consider whether headers likeAccept,Accept-Encoding, andContent-Typeshould also be included in this list, as they can affect the response and may lead to different outcomes even if the URL and other parameters are the same.
110-122: Optimize header processing to improve performance.In
sortedNonDynamicHeaders, you're appending headers to a slice and then sorting them. For better performance, especially with a large number of headers, consider preallocating the slice with the known capacity.Here's how you can preallocate the slice:
func sortedNonDynamicHeaders(headers mapsutil.OrderedMap[string, string]) []header { - var result []header + result := make([]header, 0, headers.Len()) headers.Iterate(func(k, v string) bool { if !dynamicHeaders[strings.ToLower(k)] { result = append(result, header{Key: k, Value: v}) } return true }) sort.Slice(result, func(i, j int) bool { return result[i].Key < result[j].Key }) return result }pkg/input/provider/http/multiformat.go (1)
30-31: Update comment to accurately describe 'InputContents'The comment currently states
// optional input reader, but the field added isInputContents string, which represents the input content as a string, not a reader. Consider updating the comment to// optional input contentsto better reflect its purpose.internal/server/server.go (5)
58-62: Avoid hardcoding default values for executor optionsThe condition checks for specific default values (
25) to adjustBulkSizeandTemplateThreads. Hardcoding these values can lead to maintenance issues if defaults change.Consider referencing the default values from the
nucleiconfiguration or defining constants to represent them:-// Disable bulk mode and single threaded execution // by auto adjusting in case of default values -if options.NucleiExecutorOptions.Options.BulkSize == 25 && options.NucleiExecutorOptions.Options.TemplateThreads == 25 { +defaultBulkSize := nuclei.DefaultBulkSize +defaultTemplateThreads := nuclei.DefaultTemplateThreads +if options.NucleiExecutorOptions.Options.BulkSize == defaultBulkSize && options.NucleiExecutorOptions.Options.TemplateThreads == defaultTemplateThreads { options.NucleiExecutorOptions.Options.BulkSize = 1 options.NucleiExecutorOptions.Options.TemplateThreads = 1 }Replace
nuclei.DefaultBulkSizeandnuclei.DefaultTemplateThreadswith the actual constants from the nuclei package.
170-173: Handle task submission errors inhandleRequestWhen submitting tasks to the
tasksPool, errors may occur if the pool is stopped or overloaded. Currently, errors are not being handled, which could lead to unacknowledged failures.Modify the code to handle potential errors:
- s.tasksPool.Submit(func() { + if err := s.tasksPool.Submit(func() { s.consumeTaskRequest(req) - }) + }); err != nil { + return c.JSON(500, map[string]string{"error": "failed to submit task"}) + } return c.NoContent(200) }This ensures that the client is informed if the task could not be submitted.
166-168: Enhance input validation forhandleRequestCurrently, the validation only checks for empty fields. Additional validation can prevent processing invalid data.
Consider validating that
req.URLis a well-formed URL and thatreq.RawHTTPcontains valid HTTP request data. For example:import "net/url" // Validate the request if req.RawHTTP == "" || req.URL == "" { return c.JSON(400, map[string]string{"error": "missing required fields"}) } +if _, err := url.ParseRequestURI(req.URL); err != nil { + return c.JSON(400, map[string]string{"error": "invalid URL format"}) +}This will help prevent errors during task processing due to invalid inputs.
127-129: Reconsider skipping authentication for/statsendpointThe
Skipperfunction excludes the/statsendpoint from authentication. If the statistics contain sensitive information, it may be prudent to require authentication.Evaluate whether the data returned by
/statsshould be publicly accessible. If not, modify theSkipperfunction:Skipper: func(c echo.Context) bool { - return c.Path() == "/stats" + return false },This change will enforce authentication on all endpoints.
139-141: Serve UI files securelyThe
/uiendpoint serves a static HTML file frominternal/server/ui/index.html. Ensure that the file path is secure to prevent directory traversal attacks.[security_issue]
Use
echo.Staticor ensure the file path is a constant:e.GET("/ui", func(c echo.Context) error { - return c.File("internal/server/ui/index.html") + return c.File(filepath.Join("internal", "server", "ui", "index.html")) })Also, consider using
Embedto embed static files within the binary for added security.pkg/authprovider/authx/dynamic.go (1)
Line range hint
53-62: Fix infinite recursion inUnmarshalJSONmethodThe
UnmarshalJSONmethod callsjson.Unmarshal(data, &d), which causes infinite recursion because it triggersUnmarshalJSONagain. This will result in a stack overflow.Apply the following fix using an alias type to avoid recursion:
func (d *Dynamic) UnmarshalJSON(data []byte) error { - if err := json.Unmarshal(data, &d); err != nil { + type Alias Dynamic + var alias Alias + if err := json.Unmarshal(data, &alias); err != nil { return err } var s Secret if err := json.Unmarshal(data, &s); err != nil { return err } + *d = Dynamic(alias) d.Secret = &s return nil }internal/server/nuclei_sdk.go (3)
173-173: Standardize error wrapping usingerrors.WrapFor consistency, consider using
errors.Wrap(err, ...)instead offmt.Errorfwhen wrapping errors, as you've done elsewhere in the code.Apply this diff to standardize error handling:
- return fmt.Errorf("error marshalling yaml: %s", err) + return errors.Wrap(err, "error marshalling YAML")
160-169: Unused struct fields inproxifyRequest.RequestIn the
proxifyRequeststruct, the fieldsHeaderandBodyare defined but not populated when creating the payload. Consider removing these fields if they're unnecessary, or ensure they're set appropriately.Apply this diff to remove the unused fields:
type proxifyRequest struct { URL string `json:"url"` Request struct { - Header map[string]string `json:"header"` - Body string `json:"body"` Raw string `json:"raw"` } `json:"request"` }Alternatively, if these fields are needed, populate them when creating the
payload:payload := proxifyRequest{ URL: target.URL, Request: struct { Header map[string]string `json:"header"` Body string `json:"body"` Raw string `json:"raw"` }{ + Header: target.Headers, + Body: target.Body, Raw: target.RawHTTP, }, }
191-199: Handle errors returned byClose()methodsThe errors returned by
Close()methods are currently ignored. Handling these errors can help catch issues during resource cleanup.Apply this diff to handle the errors:
if n.executorOpts.FuzzStatsDB != nil { - n.executorOpts.FuzzStatsDB.Close() + if err := n.executorOpts.FuzzStatsDB.Close(); err != nil { + // Handle or log the error as appropriate + } } if n.options.Interactsh != nil { - _ = n.options.Interactsh.Close() + if err := n.options.Interactsh.Close(); err != nil { + // Handle or log the error as appropriate + } } if n.executorOpts.InputHelper != nil { - _ = n.executorOpts.InputHelper.Close() + if err := n.executorOpts.InputHelper.Close(); err != nil { + // Handle or log the error as appropriate + } }internal/runner/runner.go (2)
451-473: Rename variableexecurOptstoexecutorOptsfor consistencyThe variable
execurOptsappears to be a typo. Renaming it toexecutorOptsimproves readability and adheres to standard naming conventions.Apply this diff to correct the variable name:
- execurOpts := &server.NucleiExecutorOptions{ + executorOpts := &server.NucleiExecutorOptions{ Options: r.options, Output: r.output, Progress: r.progress, Catalog: r.catalog, IssuesClient: r.issuesClient, RateLimiter: r.rateLimiter, Interactsh: r.interactsh, ProjectFile: r.projectFile, Browser: r.browser, Colorizer: r.colorizer, Parser: r.parser, TemporaryDirectory: r.tmpDir, } - dastServer, err := server.New(&server.Options{ ... - NucleiExecutorOptions: execurOpts, + NucleiExecutorOptions: executorOpts, })
525-529: Avoid redeclaring theerrvariable to prevent shadowingThe variable
erris redeclared within the function scope, which could lead to variable shadowing and potential bugs. Removing the redundant declaration ensures clarity.Apply this diff to eliminate the unnecessary declaration:
- var err error executorOpts.FuzzStatsDB, err = fuzzStats.NewTracker(r.options.DASTScanName)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
cmd/nuclei/main.go(4 hunks)go.mod(5 hunks)internal/runner/lazy.go(1 hunks)internal/runner/runner.go(7 hunks)internal/server/dedupe.go(1 hunks)internal/server/nuclei_sdk.go(1 hunks)internal/server/requests_worker.go(1 hunks)internal/server/scope/extensions.go(1 hunks)internal/server/scope/scope.go(1 hunks)internal/server/scope/scope_test.go(1 hunks)internal/server/server.go(1 hunks)pkg/authprovider/authx/cookies_auth.go(1 hunks)pkg/authprovider/authx/dynamic.go(6 hunks)pkg/authprovider/authx/strategy.go(1 hunks)pkg/authprovider/file.go(2 hunks)pkg/catalog/loader/loader.go(1 hunks)pkg/fuzz/component/component.go(1 hunks)pkg/fuzz/component/cookie.go(1 hunks)pkg/fuzz/stats/db.go(1 hunks)pkg/fuzz/stats/db_test.go(1 hunks)pkg/fuzz/stats/schema.sql(1 hunks)pkg/fuzz/stats/stats.go(1 hunks)pkg/input/formats/burp/burp.go(2 hunks)pkg/input/formats/burp/burp_test.go(2 hunks)pkg/input/formats/formats.go(2 hunks)pkg/input/formats/json/json.go(1 hunks)pkg/input/formats/json/json_test.go(2 hunks)pkg/input/formats/openapi/openapi.go(2 hunks)pkg/input/formats/openapi/openapi_test.go(2 hunks)pkg/input/formats/swagger/swagger.go(2 hunks)pkg/input/formats/swagger/swagger_test.go(2 hunks)pkg/input/formats/yaml/multidoc.go(1 hunks)pkg/input/formats/yaml/multidoc_test.go(2 hunks)pkg/input/provider/http/multiformat.go(4 hunks)pkg/protocols/http/request.go(3 hunks)pkg/protocols/protocols.go(2 hunks)pkg/types/types.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/fuzz/component/component.go
🧰 Additional context used
🪛 GitHub Check: Lint
cmd/nuclei/main.go
[failure] 225-225:
var nameGenerator is unused (unused)
pkg/fuzz/stats/db.go
[failure] 80-80:
Error return value of tx.Rollback is not checked (errcheck)
[failure] 114-114:
Error return value of tx.Rollback is not checked (errcheck)
pkg/fuzz/stats/stats.go
[failure] 57-57:
Error return value of t.database.InsertMatchedRecord is not checked (errcheck)
[failure] 62-62:
Error return value of t.database.InsertComponent is not checked (errcheck)
pkg/input/formats/swagger/swagger.go
[failure] 50-50:
ineffectual assignment to err (ineffassign)
🪛 golangci-lint (1.62.2)
cmd/nuclei/main.go
225-225: var nameGenerator is unused
(unused)
🔇 Additional comments (59)
pkg/input/formats/yaml/multidoc_test.go (2)
4-4: LGTM!
The addition of the os package import is appropriate for the new file handling implementation.
26-29: Verify consistency with other format implementations.
The update to accept an io.Reader instead of a file path is a good improvement. However, let's verify that this pattern is consistently implemented across other format parsers.
✅ Verification successful
Parse method implementation is consistent across all format implementations
The verification shows that all format implementations (YAML, JSON, Burp, Swagger, OpenAPI) consistently implement the Format interface with the correct Parse method signature:
- All implementations accept
io.Readeras the first parameter - Use
ParseReqRespCallbackfor the callback function - Include the
filePathstring parameter - Return an error
The change in the YAML implementation aligns perfectly with the interface requirements and other implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Parse method signature consistency across format implementations
# Check Parse method signatures in all format implementations
ast-grep --pattern 'func $_.Parse($reader io.Reader, $callback func($_ *types.RequestResponse) bool, $_ string)'
# Check test implementations for consistency
rg -l "os.Open.*Parse.*defer.*Close" pkg/input/formats/*/
Length of output: 307
Script:
#!/bin/bash
# Let's try a different approach to find Parse method implementations
# Find all Parse method implementations in format packages
rg -A 3 "^func.*Parse" pkg/input/formats/
# Also check interface definition
rg "type.*Format.*interface" pkg/input/formats/
Length of output: 3569
Script:
#!/bin/bash
# Let's check the Format interface definition to verify the expected signature
rg -A 5 "type Format interface" pkg/input/formats/formats.go
# Also check the ParseReqRespCallback type definition
rg "type ParseReqRespCallback" pkg/input/formats/formats.go
Length of output: 451
pkg/input/formats/burp/burp_test.go (3)
4-4: LGTM!
The addition of the "os" import is necessary for the new file handling implementation.
18-21: Well-implemented file handling!
Good practices observed:
- Proper error handling with descriptive message
- Immediate deferred close after successful open
- Clear variable naming
18-25: Verify consistent implementation across other format parsers.
Let's ensure this file handling pattern is consistently implemented across other format parsers.
✅ Verification successful
Implementation is consistent across format parsers
The verification shows consistent implementation across all format parsers:
- All Parse functions use
io.Readerinterface in their signatures - All test files follow the same pattern of using
os.Open()with properdefer file.Close() - The callback function signature
ParseReqRespCallbackis consistently used across all implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other format parsers follow the same pattern
# Expected: Similar file handling pattern in other format test files
# Search for test files in the formats directory
fd -e go -e test.go . pkg/input/formats/ --exec rg -l 'func Test.*Parse'
# Check the implementation pattern in these files
rg -A 5 'os\.Open.*Parse' pkg/input/formats/
Length of output: 317
Script:
#!/bin/bash
# Let's check the implementation pattern in test files more thoroughly
# First, let's look at the burp test implementation
echo "=== Burp Test Implementation ==="
rg -B 2 -A 5 'Parse.*func.*RequestResponse.*bool' pkg/input/formats/burp/burp_test.go
# Now check other test implementations
echo -e "\n=== Other Test Implementations ==="
rg -B 2 -A 5 'Parse.*func.*RequestResponse.*bool' pkg/input/formats/
# Also check the actual Parse function signatures in the implementation files
echo -e "\n=== Parse Function Signatures ==="
ast-grep --pattern 'func $_ Parse($$$) $$$'
Length of output: 4041
Script:
#!/bin/bash
# Let's check the actual Parse function implementations
echo "=== Parse Function Implementations ==="
# First check burp implementation
echo "Burp Parse Implementation:"
rg -B 2 -A 5 '^func.*Parse\(' pkg/input/formats/burp/burp.go
# Then check other implementations
echo -e "\nOther Parse Implementations:"
rg -B 2 -A 5 '^func.*Parse\(' pkg/input/formats/*/[^_]*.go
# Also check if there are any other file opening patterns
echo -e "\n=== File Opening Patterns ==="
rg 'os\.Open.*' pkg/input/formats/
Length of output: 6223
internal/server/scope/scope_test.go (2)
1-9: LGTM! Package and imports are well-structured.
The imports are appropriate for the test functionality, using standard testing package and required external dependencies.
20-24: Enhance test coverage and verify domain handling.
The test demonstrates cross-domain validation but could be improved:
- Add test cases for edge cases (empty paths, special characters)
- Verify that domain handling is working as intended
- Document why different domains are used in the validation
pkg/input/formats/swagger/swagger_test.go (1)
22-25: Consider enhancing error handling and clarifying parameter usage.
The switch to io.Reader improves the design by standardizing input handling. However, there are two potential improvements:
- Consider using require.NoError for consistent error handling:
-if err != nil {
- t.Fatal(err)
-}
+require.NoError(t, err, "failed to parse swagger file")- The file path parameter might need clarification:
✅ Verification successful
Consider using require.NoError for consistent error handling
The switch to io.Reader improves the design. The filePath parameter is actually used meaningfully within the Parse implementation to determine the file extension and choose the appropriate parsing strategy (YAML vs JSON), so it's not redundant. However, you can improve the test's error handling:
-if err != nil {
- t.Fatal(err)
-}
+require.NoError(t, err, "failed to parse swagger file")🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the filePath parameter is used for anything other than error reporting
ast-grep --pattern 'func Parse($_: io.Reader, $_: func(*types.RequestResponse) bool, filePath string) error'
Length of output: 110
Script:
#!/bin/bash
# Let's first find the swagger format parser implementation
fd "swagger.*\.go" --type f --exec cat {}
Length of output: 2777
pkg/authprovider/authx/strategy.go (1)
27-37: Verify concurrent access safety
Since this code handles authentication strategies that could be accessed concurrently, we should verify thread safety.
Consider these architectural recommendations:
- Document thread safety requirements in the interface comments
- Consider using atomic operations or mutex if concurrent access is possible
- Add unit tests specifically for concurrent strategy application
✅ Verification successful
Let me gather more information about the concurrent usage patterns and strategy implementation.
Let me check one more aspect to understand how the strategies are initialized and managed.
Thread safety is properly implemented
The code is already thread-safe with appropriate synchronization:
- The
Dynamicstruct has async.Mutexfield for synchronizing concurrent access - The
Fetchmethod uses proper lock/unlock mechanisms - Strategy retrieval through
GetStrategies()is protected by the fetching mechanism - Strategies are immutable after initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for concurrent access patterns to Dynamic and its strategies
ast-grep --pattern 'go func() { $$$ d.Dynamic $$$ }'
# Look for any mutex usage in Dynamic type
rg -A 5 'type Dynamic struct'
Length of output: 748
Script:
#!/bin/bash
# Check GetStrategies implementation and usage
ast-grep --pattern 'func ($x *Dynamic) GetStrategies() $ret { $$$ }'
# Look for goroutine usage in files containing Dynamic or Strategy
rg -l "Dynamic|Strategy" | xargs rg "go \w+|go func"
# Check if strategies are modified after initialization
ast-grep --pattern '$x.SetStrategies($y)'
# Look for any concurrent operations in test files
fd _test.go --exec rg -A 3 "go func|parallel|sync\."
Length of output: 1717
Script:
#!/bin/bash
# Check Dynamic struct methods and initialization
rg -A 10 "func.*Dynamic" pkg/authprovider/authx/
# Look for strategy initialization and modification patterns
ast-grep --pattern 'func ($x *Dynamic) $method($params) { $$$ }'
# Check if there are any locks or synchronization mechanisms
rg -w "sync\.|Mutex|RWMutex|Lock|Unlock" pkg/authprovider/authx/
Length of output: 7094
pkg/authprovider/authx/cookies_auth.go (1)
Line range hint 23-46: Inconsistency between Apply and ApplyOnRR methods
The Apply and ApplyOnRR methods now handle cookies differently. Apply appends cookies while ApplyOnRR clears them first. This inconsistency could lead to bugs depending on which method is called.
Either:
- Update both methods to have consistent behavior
- Document why they need to behave differently
Let's verify the usage of these methods:
✅ Verification successful
Let me gather more context about these usages by checking the surrounding code.
From the context gathered, I can now generate the final response as the implementation details reveal the architectural intent.
The different cookie handling behavior between Apply and ApplyOnRR is correct by design
The implementation shows that:
ApplyandApplyOnRRare part of theAuthStrategyinterface, used in different contextsApplyOnRRis specifically used inbuild_request.gowhere multiple auth strategies are applied sequentially, making cookie cleanup necessary to prevent strategy conflictsApplyis used in a simpler context through theDynamicAuthStrategywhere cookies can be accumulated
The current implementation is correct as:
- Clearing cookies in
ApplyOnRRensures clean state when building requests with multiple auth strategies - Appending cookies in
Applyallows for simpler, direct HTTP request modification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of Apply and ApplyOnRR to understand the impact
echo "Searching for Apply usage:"
rg -A 2 "\.Apply\([^R]"
echo "Searching for ApplyOnRR usage:"
rg -A 2 "\.ApplyOnRR\("
Length of output: 590
Script:
#!/bin/bash
# Check the implementation of both methods in strategy.go
echo "Checking strategy.go implementation:"
rg -B 5 -A 5 "Apply.*\(" pkg/authprovider/authx/strategy.go
# Check the build_request.go implementation
echo "Checking build_request.go implementation:"
rg -B 5 -A 5 "ApplyOnRR" pkg/protocols/http/build_request.go
# Check if there are any other cookie-related operations in build_request.go
echo "Checking cookie operations in build_request.go:"
rg -B 2 -A 2 "Cookie" pkg/protocols/http/build_request.go
Length of output: 1668
pkg/input/formats/openapi/openapi.go (2)
4-5: LGTM: Import changes are appropriate
The addition of the io package import is necessary for the new io.Reader parameter and follows Go's import grouping conventions.
34-34: LGTM: Improved input handling flexibility
The signature change from string to io.Reader enhances the parser's flexibility, making it suitable for live DAST server implementation by supporting streaming and in-memory inputs.
Let's verify that all format implementations consistently implement the new interface:
✅ Verification successful
LGTM: Interface implementation consistency verified
All format implementations (OpenAPI, JSON, Swagger, YAML MultiDoc, and Burp) correctly implement the Format interface with the updated Parse method signature using io.Reader and filePath parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent Parse method signatures across format implementations
# Expected: All format implementations should use io.Reader and include filePath parameter
ast-grep --pattern 'type Format interface {
$$$
Parse(input io.Reader, $_, filePath string) error
$$$
}'
# Verify all implementations
ast-grep --pattern 'func ($_) Parse(input io.Reader, $_, filePath string) error'
Length of output: 195
Script:
#!/bin/bash
# Let's try a different approach to find the Format interface and its implementations
# Find the Format interface definition
echo "=== Format Interface Definition ==="
rg -A 5 "type Format interface" --type go
# Find all Parse method implementations
echo -e "\n=== Parse Method Implementations ==="
rg "^func.*Parse.*io\.Reader.*error" --type go
Length of output: 1585
pkg/fuzz/stats/db_test.go (1)
1-7: LGTM!
The package name and imports are appropriate for a test file.
internal/server/scope/extensions.go (2)
1-4: LGTM: Package structure and imports are appropriate.
The package name aligns with the directory structure, and the path import is suitable for the file extension handling functionality.
1-33: Verify alignment with DAST best practices.
Let's verify if this implementation aligns with other DAST tools and security testing best practices.
✅ Verification successful
Extension filtering aligns with security scanning best practices
The implementation is correctly used as a security measure in the request processing pipeline. From the context gathered:
- It's used in
requests_worker.goto filter out uninteresting paths before scope validation, preventing unnecessary processing of static/binary files - The filtering happens early in the request processing chain, which is an efficient security practice
- The extensive list of excluded extensions covers common static, binary, and media files that typically don't need security scanning
- Similar extension filtering patterns are also implemented in the file protocol (
pkg/protocols/file/file.go), showing consistency in the approach
The implementation aligns with DAST best practices by focusing scanning efforts on potentially vulnerable endpoints while excluding static content that typically doesn't pose security risks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar extension filtering in other files
echo "Searching for similar extension filtering patterns..."
rg -g '!{vendor,node_modules}/*' -i 'extensi.*map.*struct'
# Check if there are any existing DAST-related comments or documentation
echo "Checking for DAST-related documentation..."
fd -g '*.md' -x grep -l -i 'dast|dynamic.*security.*test|extension.*filter'
Length of output: 575
Script:
#!/bin/bash
# Let's check the context of file.go's extension usage
echo "Checking file.go extension handling..."
rg -A 10 -B 10 'extensions.*map\[string\]struct{}' pkg/protocols/file/file.go
# Let's also check for any documentation about file filtering or scanning
echo "Checking for file filtering documentation..."
rg -i "file.*filter|scan.*file|skip.*file" -g "*.md" -g "*.go"
# Let's see the broader context of how these extensions are used
echo "Checking usage of IsUninterestingPath..."
rg "IsUninterestingPath"
Length of output: 4223
Script:
#!/bin/bash
# Let's check how the IsUninterestingPath is used in the request worker
echo "Checking requests_worker.go context..."
rg -A 10 -B 10 "IsUninterestingPath" internal/server/requests_worker.go
# Let's check if there are any configuration files or documentation about scanning strategy
echo "Checking for scanning strategy documentation..."
fd -g "*.go" -x grep -l -i "scan.*strateg|filter.*path|exclude.*path"
# Let's see the broader context of file extensions in protocols
echo "Checking file protocol implementation..."
rg -A 5 -B 5 "extensions\s*=" pkg/protocols/file/file.go
Length of output: 1347
pkg/input/formats/burp/burp.go (1)
5-5: LGTM: Import addition is appropriate
The addition of the io.Reader import aligns with the method signature change and follows Go import conventions.
pkg/input/formats/openapi/openapi_test.go (2)
4-4: LGTM: Import addition is appropriate
The addition of the "os" import is necessary for the new file handling implementation.
49-53: Verify consistent implementation across other format parsers
The change to accept an io.Reader in the Parse method is a good improvement. Let's verify this pattern is consistently implemented across other format parsers.
✅ Verification successful
Pattern is consistently implemented across all format parsers
The verification shows that all format parsers consistently implement the same Parse method signature with io.Reader. This is evident in:
- The interface definition in
formats.go:Parse(input io.Reader, resultsCb ParseReqRespCallback, filePath string) error - Implementations in all format parsers:
JSONFormat.ParseYamlMultiDocFormat.ParseBurpFormat.ParseSwaggerFormat.ParseOpenAPIFormat.Parse
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other format parsers follow the same pattern
# Expected: All Parse methods should accept io.Reader
# Search for Parse method declarations in format packages
ast-grep --pattern 'type $_ interface {
$$$
Parse($reader, $_, $_) $_
$$$
}'
# Search for Parse implementations
ast-grep --pattern 'func ($_) Parse($reader, $_, $_) $_ {
$$$
}'
Length of output: 153
Script:
#!/bin/bash
# Let's try a different approach to find Parse implementations and interfaces
# Find all files that might contain format parsers
fd "format|parser" "pkg/input"
# Search for Parse method signatures
rg "Parse\(" "pkg/input" -A 2
# Search specifically in the formats directory
rg "Parse\(" "pkg/input/formats" -A 2
Length of output: 9016
pkg/input/formats/json/json.go (2)
48-49: LGTM! Good improvement in the interface design.
The change from string to io.Reader is a solid improvement that:
- Provides more flexibility in input sources
- Follows Go's io interfaces pattern
- Enables better testing and memory efficiency
48-48: Verify interface consistency across format parsers.
Let's ensure this interface change is consistently implemented across all format parsers mentioned in the summary.
✅ Verification successful
Interface implementation is consistent across all format parsers
Based on the verification results, all format parsers (Burp, JSON, OpenAPI, Swagger, and YAML) consistently implement the Parse method with the exact same signature as defined in the Format interface:
Parse(input io.Reader, resultsCb formats.ParseReqRespCallback, filePath string) error🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in Parse method signatures across format parsers
# Search for Parse method signatures in format parsers
ast-grep --pattern 'func $_($_ *$_) Parse(input io.Reader, $$$) error' pkg/input/formats/
Length of output: 91
Script:
#!/bin/bash
# Let's try a different approach to find all Parse method implementations
# First, let's find all format parser files
fd . pkg/input/formats/ -t f -e go | while read -r file; do
echo "=== $file ==="
# Show the Parse method implementation with context
rg "func.*Parse.*io\.Reader" -A 2 "$file"
done
Length of output: 3297
Script:
#!/bin/bash
# Let's check the interface definition to ensure we're matching the correct signature
rg "type.*interface" -A 5 pkg/input/formats/formats.go
Length of output: 320
pkg/input/formats/swagger/swagger.go (2)
10-10: LGTM!
The yaml package import is appropriate for the YAML parsing functionality added to the code.
41-41: Verify all callers of the Parse method have been updated
The signature change from Parse(input string, ...) to Parse(input io.Reader, ..., filePath string) is a breaking change.
Let's verify all callers have been updated:
pkg/input/formats/yaml/multidoc.go (1)
48-48: LGTM: Interface change improves flexibility
The change from string to io.Reader is a good improvement as it:
- Provides more flexibility in input sources
- Allows for streaming large files
- Follows Go's standard io interfaces pattern
pkg/input/formats/json/json_test.go (3)
4-4: LGTM: Import addition is appropriate
The addition of the "os" import is necessary for the new file handling implementation.
45-48: LGTM: Robust file handling implementation
The new file handling implementation is well-structured with:
- Proper error handling using require.Nilf
- Resource cleanup with defer
- Descriptive error message
50-53: Verify consistent updates across test files
Since the Parse method signature has been changed to accept io.Reader across different format packages, we should verify that all related test files have been updated consistently.
✅ Verification successful
All format implementations consistently use io.Reader
Based on the verification results, all format implementations (burp.go, json.go, openapi.go, swagger.go, yaml/multidoc.go) consistently use the same Parse method signature with io.Reader. The test files across different formats also show consistent usage patterns. No inconsistencies were found.
- All implementations follow:
Parse(input io.Reader, resultsCb formats.ParseReqRespCallback, filePath string) error - All test files use the same pattern for calling
format.Parse(file, callback, filepath)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar test files that might need the same update
fd -e go -t f "_test" "pkg/input/formats" --exec grep -l "format.Parse"
# Look for potentially inconsistent Parse usage
rg "format\.Parse\(.*string.*\)" "pkg/input/formats"
Length of output: 316
Script:
#!/bin/bash
# Let's examine the Parse method usage in all test files to verify consistency
for file in pkg/input/formats/*/[^.]*.go; do
echo "=== $file ==="
rg "Parse\(" -A 2 "$file"
done
# Also check the Parse method signatures in the implementation files
for file in pkg/input/formats/*/[^.]*_test.go; do
echo "=== $file ==="
ast-grep --pattern 'format.Parse($$$)'
done
Length of output: 5319
internal/server/scope/scope.go (1)
42-57: Consider URL normalization for security
To prevent bypass attempts, consider normalizing URLs before validation. This helps handle various URL encoding schemes and prevents scope bypass through URL manipulation.
Let's check if URL normalization is handled elsewhere in the codebase:
pkg/fuzz/stats/stats.go (2)
1-11: LGTM! Clean package structure with clear documentation.
13-29: Verify StatsDatabase interface implementation
The code references a StatsDatabase interface that's not defined in this file. We should verify that all required interface methods are properly implemented by sqliteStatsDatabase.
✅ Verification successful
Let me verify if the sqliteStatsDatabase implements all the required methods from the StatsDatabase interface.
Interface implementation is complete and correct
The sqliteStatsDatabase struct correctly implements all methods required by the StatsDatabase interface:
Close()InsertComponent(event FuzzingEvent) errorInsertMatchedRecord(event FuzzingEvent) error
All method signatures match the interface requirements and contain proper implementations with error handling and transaction management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for StatsDatabase interface definition and implementation
ast-grep --pattern 'type StatsDatabase interface {
$$$
}'
# Search for sqliteStatsDatabase struct and its methods
ast-grep --pattern 'type sqliteStatsDatabase struct {
$$$
}'
Length of output: 820
Script:
#!/bin/bash
# Search for sqliteStatsDatabase method implementations
ast-grep --pattern 'func (s *sqliteStatsDatabase) Close() {
$$$
}'
ast-grep --pattern 'func (s *sqliteStatsDatabase) InsertComponent(event FuzzingEvent) error {
$$$
}'
ast-grep --pattern 'func (s *sqliteStatsDatabase) InsertMatchedRecord(event FuzzingEvent) error {
$$$
}'
Length of output: 4555
pkg/fuzz/component/cookie.go (2)
Line range hint 84-95: Critical: Address cookie duplication with auth-file
The TODO comment indicates a known issue with cookie duplication when using auth-file. This could lead to:
- Incorrect authentication states
- Increased request sizes
- Potential security implications if conflicting cookies are sent
Let's analyze the auth-file handling to understand the duplication:
Line range hint 38-54: Security: Verify impact of processing all cookies
The removal of cookie filtering (defaultIgnoredCookieKeys) means all cookies are now processed during fuzzing. While this enables more comprehensive testing, it could:
- Expose sensitive cookies to fuzzing
- Impact session management
- Affect performance with unnecessary cookie processing
Let's check for sensitive cookie handling elsewhere in the codebase:
✅ Verification successful
Security: Cookie component changes are safe to proceed
The removal of cookie filtering is intentional and does not introduce security concerns because:
- The cookie component is part of the fuzzing framework where comprehensive testing is desired
- The test file
fuzz-cookie-error-sqli.yamlshows this is used for security testing (SQL injection) - Cookie security is handled at the HTTP client level with proper cookie jar management
- Sensitive cookie handling is managed by the application using the fuzzer, not the fuzzer itself
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for sensitive cookie handling patterns
rg -A 5 "sensitive.*cookie|cookie.*sensitive"
# Check for security-related cookie configurations
ast-grep --pattern 'cookie {
$$$
secure
$$$
}'
Length of output: 102
Script:
#!/bin/bash
# Look for cookie-related security configurations and filtering patterns
rg -A 5 "defaultIgnoredCookieKeys|ignoredCookies|filterCookies|skipCookies"
# Search for cookie-related security implementations
rg -A 5 "cookie.*security|security.*cookie"
# Look for cookie handling in the component
fd "cookie\.go" --exec cat {}
# Check for cookie-related tests that might reveal security considerations
fd "cookie.*test\.go" --exec cat {}
Length of output: 4085
Script:
#!/bin/bash
# Look for previous versions of the cookie component to understand what was removed
git log -p -- pkg/fuzz/component/cookie.go
# Search for any cookie filtering or security measures in the fuzzing package
fd -e go -e yaml -e yml . | xargs rg -l "fuzz.*cookie|cookie.*fuzz"
# Look for any cookie-related configurations in the wider codebase
rg -A 5 "cookie.*config|config.*cookie"
Length of output: 601
Script:
#!/bin/bash
# Check the fuzzing test file content
cat integration_tests/fuzz/fuzz-cookie-error-sqli.yaml
# Look at the fuzzing implementation
cat pkg/fuzz/fuzz.go
# Check HTTP request handling for cookie-related security
cat pkg/protocols/http/request.go
Length of output: 54477
pkg/input/formats/formats.go (2)
5-5: LGTM: Import addition is appropriate
The addition of the "io" import is necessary to support the new io.Reader parameter in the Parse method signature.
39-39: Verify all Format interface implementations are updated
The change from string to io.Reader is a good improvement that allows for more efficient streaming of inputs and better memory management. However, this is a breaking change to the Format interface.
Let's verify all implementations have been updated:
✅ Verification successful
All Format interface implementations are correctly updated
All implementations of the Format interface have been updated to use the new Parse method signature with io.Reader and filePath string parameters:
- YamlMultiDocFormat in pkg/input/formats/yaml/multidoc.go
- OpenAPIFormat in pkg/input/formats/openapi/openapi.go
- SwaggerFormat in pkg/input/formats/swagger/swagger.go
- BurpFormat in pkg/input/formats/burp/burp.go
- JSONFormat in pkg/input/formats/json/json.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of the Format interface and verify they've been updated
# to use the new Parse method signature
# Search for all types that implement the Format interface
ast-grep --pattern 'type $name struct {
$$$
}
func ($receiver $name) Name() string {
$$$
}
func ($receiver $name) Parse($$$) error {
$$$
}
func ($receiver $name) SetOptions($$$) {
$$$
}'
# Additionally search for any old Parse method signatures that might need updating
rg "Parse\(input string, resultsCb ParseReqRespCallback\) error"
Length of output: 266
Script:
#!/bin/bash
# Let's try a different approach to find Format interface implementations
# First, find the Format interface definition file
fd "formats.go"
# Then find all struct types that might implement Format
rg "type \w+ struct" -A 3 pkg/input/formats/
# Search for Parse method implementations with context
rg "func .+ Parse\(" -A 3 pkg/input/formats/
Length of output: 4443
internal/runner/lazy.go (2)
116-124: Consider DAST security implications
As this code is part of a DAST server implementation handling authentication and value extraction, consider the following security aspects:
- Value sanitization before storage
- Protection against injection attacks in extracted values
- Secure handling of sensitive authentication data
- Rate limiting for template execution
- Audit logging of value changes
Consider implementing:
- Value sanitization layer before storage
- Rate limiting mechanism for template execution
- Audit logging for security-sensitive operations
- Secure storage for sensitive extracted values
Let's check for existing security measures:
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Security measures are already in place
The codebase already implements several key security controls:
-
Input validation and sanitization:
- Extensive validation framework using
validator.New() - Path and URL sanitization functions
- Template validation checks
- Extensive validation framework using
-
Rate limiting:
- Global rate limiting implementation via
ratelimit.New() - Configurable rate limits with duration control
- Per-protocol rate limiting enforcement
- Global rate limiting implementation via
-
Logging:
- Structured logging with slog
- Error logging and validation warnings
- Audit trail for template validation
The value extraction code is protected by these existing security measures, making the original review comment's concerns already addressed by the codebase's security architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for sanitization functions
rg -g '!{vendor,test}/**' 'sanitize|validate|escape'
# Check for rate limiting implementations
rg -g '!{vendor,test}/**' 'RateLimit|rate.Limit'
# Look for audit logging
rg -g '!{vendor,test}/**' 'audit|log\.'
Length of output: 33899
117-123: 🛠️ Refactor suggestion
Review the longest-value selection strategy
The current implementation assumes that longer values are more significant, which may not always be the case. This could potentially:
- Miss important shorter values that might be more relevant
- Not fully address the cookie duplication issue mentioned in the PR description
- Lead to unexpected behavior if malformed or malicious values are present
Let's verify the impact of this change on cookie handling:
Consider enhancing the value selection logic:
for _, value := range v {
oldVal, ok := data[k]
- if !ok || len(value) > len(oldVal.(string)) {
+ if !ok {
data[k] = value
+ } else {
+ // Add validation for the value
+ if !isValidValue(value) {
+ continue
+ }
+ // Consider additional criteria beyond just length
+ if shouldReplaceValue(oldVal.(string), value) {
+ data[k] = value
+ }
}
}pkg/authprovider/file.go (2)
Line range hint 88-120: Ensure comprehensive security testing coverage
Given that this code is part of a DAST server implementation and handles authentication, it's crucial to have thorough security testing.
Please ensure test coverage includes:
- Race condition testing for concurrent access
- Edge cases in domain formats
- Authentication bypass attempts
- Memory leak scenarios with large numbers of dynamic secrets
88-89: Verify the implementation of GetDomainAndDomainRegex
The consolidation of domain retrieval logic into a single method is good for maintainability. However, we need to ensure proper validation is maintained.
✅ Verification successful
GetDomainAndDomainRegex implementation is correct and maintains proper validation
The implementation of GetDomainAndDomainRegex() in pkg/authprovider/authx/dynamic.go is a straightforward aggregation method that:
- Correctly collects domains from all secrets in the Dynamic struct
- Preserves both plain domains and regex domains separately
- Maintains the original validation structure as domains and domain regexes are properly defined in the Secret struct
The consolidation of domain retrieval into this method improves code organization without compromising security or validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of GetDomainAndDomainRegex
ast-grep --pattern 'func ($x *DynamicSecret) GetDomainAndDomainRegex() ($_, $_) {
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's try a broader search for GetDomainAndDomainRegex implementation
rg -A 5 "GetDomainAndDomainRegex"
# Also search for the struct definition to understand the correct type
rg "type.*Dynamic" -A 2
Length of output: 1556
Script:
#!/bin/bash
# Let's examine the Secret struct to understand the domain fields
rg "type Secret struct" -A 10
# Also check for any validation of domains in the Dynamic struct
rg "func.*Dynamic.*validate" -A 10
Length of output: 1133
pkg/protocols/protocols.go (1)
17-17: LGTM!
The import statement is properly placed with other internal imports and follows the package organization pattern.
go.mod (5)
54-54: LGTM: New dependencies align with DAST server implementation.
The addition of these dependencies is appropriate for the live DAST server implementation:
pond: Provides thread pool management for concurrent operationsnamegenerator: Handles dynamic name generationmux: Robust HTTP router for the server endpoints
Also applies to: 65-65, 75-75
82-82: LGTM: Dependency updates look good.
The updates to go-sqlite3 and gommon follow semantic versioning and are recent stable versions.
Also applies to: 354-354
Line range hint 6-6: Review and update the fixed version of requests.
The requests library version 2.26.0 is outdated and may contain security vulnerabilities.
Consider updating to the latest stable version after reviewing the changelog for breaking changes.
Line range hint 391-391: Document the impact of v3.2.0 retraction.
The retraction of v3.2.0 due to JavaScript protocol issues needs clear documentation.
Consider:
- Adding a detailed explanation in the changelog
- Providing migration guidelines for users on v3.2.0
- Documenting the JavaScript protocol limitations
✅ Verification successful
The retraction of v3.2.0 is already properly documented in go.mod
The go.mod file already contains a clear retraction notice for v3.2.0 with the reason "retract due to broken js protocol issue" using the standard Go module retraction syntax. This is the recommended way to document module retractions in Go, and it will be automatically respected by the Go toolchain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for JavaScript protocol related code
rg -l "javascript:|js:"
Length of output: 543
Script:
#!/bin/bash
# Check git history for retraction related commits and tags
git log --oneline | grep -i "retract\|v3.2.0"
# Check changelog for any mentions of v3.2.0
fd CHANGELOG -i --exec cat {}
# Check go.mod for retraction details
cat go.mod
Length of output: 17892
141-141: Verify the necessity of randdetect package.
The randdetect package is marked as indirect but might be crucial for DAST functionality.
Consider documenting the purpose of this dependency if it's essential for DAST operations.
pkg/catalog/loader/loader.go (1)
512-513: Verify the security implications of allowing global matcher templates
The change allows templates with global matchers to be loaded alongside fuzzing templates in DAST mode. This could potentially affect the security scanning behavior.
Let's analyze the usage of global matchers in the codebase:
✅ Verification successful
Based on the analysis of the code, I can now provide a final response about the security implications of global matchers:
Global matchers are a safe and intentional feature for passive template matching
Global matchers are designed to apply matchers statically across all result events from other templates. The implementation shows:
- Global matchers are explicitly handled in the template parsing and execution flow
- They are only loaded alongside fuzzing templates in DAST mode by design
- They don't execute additional requests but only match against existing results
- The feature has proper validation and security checks in place:
- Templates with global matchers are validated during loading
- Global matcher results are properly isolated and marked in the output
- The implementation prevents duplicate events when global matchers are used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security implications of global matchers in templates
# Find templates using global matchers
echo "Templates using global matchers:"
ast-grep --pattern 'globalMatchers: $_'
# Check for any security-related comments or documentation about global matchers
echo -e "\nSecurity documentation about global matchers:"
rg -i "global.*matcher.*security" -g "*.md"
Length of output: 269
Script:
#!/bin/bash
# Let's analyze the code more thoroughly with different approaches
# Check for global matchers usage in Go files
echo "Global matchers usage in Go files:"
rg -A 5 -B 5 "GlobalMatchers"
# Check for matcher-related structs and interfaces
echo -e "\nMatcher-related type definitions:"
ast-grep --pattern 'type $_ struct {
$$$
GlobalMatchers $$$
$$$
}'
# Check for security-related functions or methods involving matchers
echo -e "\nSecurity-related matcher functions:"
rg -A 3 "func.*Match.*" | grep -i "secur"
# Look for template validation or security checks
echo -e "\nTemplate validation and security checks:"
rg -A 5 "ValidateTemplate|SecurityCheck|Validate\("
Length of output: 64435
pkg/types/types.go (1)
416-427: Verify cookie handling in DAST server implementation.
The PR description mentions potential cookie duplication issues. While the configuration options look good, we should verify the cookie handling implementation in the DAST server code.
✅ Verification successful
The previous search revealed cookie handling in various parts of the codebase but not specifically in the DAST server implementation. Let's search more specifically for DAST server code and its cookie handling.
The previous search revealed the existence of a deduplicator in the DAST server. Let's examine its implementation to understand how it handles cookies and potential duplication issues.
Cookie duplication is properly handled by the DAST server implementation
The DAST server already implements proper cookie deduplication through its request deduplicator:
- The
cookieheader is explicitly listed indynamicHeadersmap and is excluded from request hash calculation - The
sortedNonDynamicHeadersfunction filters out cookie headers when generating request fingerprints - This ensures that requests with different cookies but otherwise identical parameters are treated as the same request, preventing duplicate processing
The review comment's concern about cookie duplication has been addressed by the existing implementation in internal/server/dedupe.go. The deduplication logic specifically accounts for cookies by treating them as dynamic headers that should not affect request uniqueness determination.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cookie-related code in the DAST server implementation
echo "Searching for cookie handling code..."
rg -A 5 "cookie|Cookie" --type go
# Search for potential cookie duplication patterns
echo "Checking for potential cookie duplication patterns..."
ast-grep --pattern 'append($SLICE, $COOKIE)' --lang go
Length of output: 100309
Script:
#!/bin/bash
# Search for cookie duplication handling in the DAST server implementation
echo "Searching for cookie duplication handling in DAST server..."
rg -A 5 "DAST.*Server" --type go
# Search for cookie handling in server implementation
echo "Searching for cookie handling in server implementation..."
rg -A 5 "func.*Server.*cookie|func.*cookie.*Server" --type go
# Search for cookie-related code in server package
echo "Searching for cookie-related code in server package..."
fd . -t f -e go -E "*_test.go" -x rg -l "cookie|Cookie" {} \; | grep "server"
Length of output: 6886
Script:
#!/bin/bash
# Check the deduplication logic in the server implementation
echo "Checking deduplication logic in server..."
rg -A 10 "deduplicator" internal/server/dedupe.go
# Check request handling and cookie processing
echo "Checking request handling and cookie processing..."
rg -A 10 "handleRequest|consumeTaskRequest" internal/server/requests_worker.go internal/server/server.go
# Check if there's any cookie-specific deduplication
echo "Checking cookie-specific deduplication..."
cat internal/server/dedupe.go
Length of output: 5160
internal/server/requests_worker.go (1)
47-51: Ensure correct request is passed to fuzzRequest
In consumeTaskRequest, parsedReq is the processed request object, while req is the original input. When calling s.fuzzRequest(req), you pass the original req. Confirm that fuzzRequest and ExecuteScan require the original request, not the parsed one.
Please verify that s.nucleiExecutor.ExecuteScan(req) works correctly with req of type PostRequestsHandlerRequest, and that it does not require parsedReq.
internal/server/dedupe.go (1)
65-88: Ensure URL normalization does not alter request semantics.
The normalizeURL function sorts query parameters and re-encodes the URL. This might change the semantics of the request if the order of parameters is significant or if certain characters are double-encoded.
Please verify that normalizing URLs in this manner does not affect the requests' intended behavior. You might want to test with URLs that have complex query parameters or rely on parameter ordering.
Run the following script to compare original and normalized URLs:
✅ Verification successful
Let me try a different approach to examine the URL normalization implementation and its usage:
Based on the implementation found, I can now provide a final response:
URL normalization implementation is safe and preserves request semantics
The normalizeURL function performs two safe normalizations:
- Sorts query parameters with same key while preserving all values
- Ensures path has a trailing slash when empty
These changes maintain request semantics as they:
- Don't modify parameter values or encoding
- Don't change parameter order for different keys
- Only sort multiple values for the same key, which is standard practice
- Add trailing slash only when path is empty, following HTTP standards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare original URLs with their normalized versions to ensure correctness.
# Test: Find all unique URLs and display their original and normalized forms.
fd -e go -x awk '/req\.URL\.URL/ {print $0}' {} + | while read -r line; do
original_url=$(echo "$line" | grep -oP '(?<=req\.URL\.URL, ).*(?=\))')
normalized_url=$(go run -e "package main; import (\"fmt\"; \"net/url\"; \"os\"); func main() { u, _ := url.Parse(os.Args[1]); fmt.Println(normalizeURL(u)) }" "$original_url")
echo "Original: $original_url"
echo "Normalized: $normalized_url"
echo "-----"
done
Length of output: 44822
Script:
#!/bin/bash
# Find the normalizeURL function implementation
ast-grep --pattern 'func normalizeURL($$$) {
$$$
}'
# Find all files that use normalizeURL
rg "normalizeURL" -l
# Look for test cases that might show URL normalization behavior
rg -A 5 -B 5 "TestNormalizeURL|TestHashRequest"
Length of output: 152
Script:
#!/bin/bash
# Let's examine the normalizeURL implementation directly
rg -A 15 "func normalizeURL" internal/server/dedupe.go
# Also check if there are any comments or documentation about URL normalization
rg -B 3 "normalizeURL" internal/server/dedupe.go
# Look for any URL-related test cases or examples
rg -A 10 "url.*:=.*\".*\"" internal/server/dedupe.go
Length of output: 659
pkg/authprovider/authx/dynamic.go (6)
26-27: Ensure proper nil handling for Secret and Secrets fields
Changing Secret to a pointer and adding the Secrets slice introduces the possibility of nil references. Ensure that all accesses to Secret and elements of Secrets include appropriate nil checks to prevent potential runtime panics.
38-52: GetDomainAndDomainRegex method correctly aggregates domains
The new GetDomainAndDomainRegex method efficiently combines domains and domain regex patterns from both Secret and Secrets, using sliceutil.Dedupe to remove duplicates. This enhances the management of multiple secrets.
75-86: Validate method now handles multiple secrets appropriately
The Validate method correctly iterates over Secrets to validate each one, in addition to validating Secret if it's not nil. This ensures that all secrets are properly validated.
104-117: SetLazyFetchCallback method enhances secret processing
The SetLazyFetchCallback method now applies extracted values to both Secret and each element in Secrets, ensuring that all secrets have the necessary values after fetching.
119-177: applyValuesToSecret method effectively replaces placeholders
The applyValuesToSecret method systematically replaces placeholders in headers, cookies, query parameters, and credentials with extracted values. This centralizes the logic and improves maintainability.
180-195: GetStrategies method correctly aggregates authentication strategies
The GetStrategies method now returns a slice of AuthStrategy by combining strategies from both Secret and Secrets. This change supports multiple authentication strategies as intended.
internal/server/nuclei_sdk.go (1)
128-130: Correct the typo in the error message
The same typo appears here in the error message "Could not create loadeopts." It should be "Could not create loader options." Please correct it for consistency.
Apply this diff to fix the typo:
- return nil, errors.Wrap(err, "Could not create loadeopts.")
+ return nil, errors.Wrap(err, "Could not create loader options.")internal/runner/runner.go (2)
448-481: Verify that early return in DAST server mode does not skip necessary initializations
Returning early from RunEnumeration() when r.options.DASTServer is true may bypass subsequent initialization steps that could be essential for the DAST server operation. Please verify that all required initializations are performed before the early return.
470-470: Ensure secure handling of the DAST server token
The DAST server uses r.options.DASTServerToken for authentication. Verify that the token is securely validated within the server to prevent unauthorized access and potential security vulnerabilities.
cmd/nuclei/main.go (3)
189-192: Verify immediate exit in DAST server mode
Upon receiving an interrupt signal, the application exits immediately in DAST server mode, potentially bypassing standard graceful shutdown procedures. Verify that this immediate exit is intended and that no essential cleanup operations are being skipped.
369-372: New DAST server flags are correctly implemented
The new command-line flags for DAST server functionality are properly added and configured as expected.
376-377: New fuzzing scope flags are correctly implemented
The flags --fuzz-scope and --fuzz-out-scope are appropriately added to define in-scope and out-of-scope URL regexes for the fuzzer.
| manager, err := NewManager([]string{`example`}, []string{`logout\.php`}) | ||
| require.NoError(t, err, "could not create scope manager") | ||
|
|
||
| parsed, _ := urlutil.Parse("https://test.com/index.php/example") | ||
| validated, err := manager.Validate(parsed.URL, "test.com") | ||
| require.NoError(t, err, "could not validate url") | ||
| require.True(t, validated, "could not get correct in-scope validation") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and test documentation.
Several improvements could be made to enhance the test's robustness and maintainability:
- URL parsing errors should not be ignored
- Add comments explaining the test scenario
- Consider using constants for test domains and patterns
+ // Test case: URL containing 'example' in path should be in scope
manager, err := NewManager([]string{`example`}, []string{`logout\.php`})
require.NoError(t, err, "could not create scope manager")
- parsed, _ := urlutil.Parse("https://test.com/index.php/example")
+ parsed, err := urlutil.Parse("https://test.com/index.php/example")
+ require.NoError(t, err, "could not parse URL")Committable suggestion skipped: line range outside the PR's diff.
| strategy := d.Dynamic.GetStrategies() | ||
| for _, s := range strategy { | ||
| s.Apply(req) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding nil checks and error handling
While the implementation correctly handles multiple strategies, there are a few considerations:
- The
strategyslice could be nil - Individual strategies in the slice could be nil
- No error handling if a strategy application fails
Consider applying this safer implementation:
- strategy := d.Dynamic.GetStrategies()
- for _, s := range strategy {
- s.Apply(req)
+ strategies := d.Dynamic.GetStrategies()
+ if strategies == nil {
+ return
+ }
+ for _, s := range strategies {
+ if s == nil {
+ continue
+ }
+ s.Apply(req)📝 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.
| strategy := d.Dynamic.GetStrategies() | |
| for _, s := range strategy { | |
| s.Apply(req) | |
| strategies := d.Dynamic.GetStrategies() | |
| if strategies == nil { | |
| return | |
| } | |
| for _, s := range strategies { | |
| if s == nil { | |
| continue | |
| } | |
| s.Apply(req) |
| // Before adding new cookies, remove existing cookies | ||
| req.Header.Del("Cookie") | ||
|
|
There was a problem hiding this comment.
Consider the implications of clearing all cookies
While removing existing cookies prevents duplication, this approach might be too aggressive as it removes ALL cookies, including those that might be essential for maintaining other aspects of the session state.
Consider these alternatives:
- Only remove cookies that will be replaced
- Merge new cookies with existing ones, updating values for matching names
Example implementation for selective cookie removal:
- req.Header.Del("Cookie")
+ // Only remove cookies that we're about to set
+ existingCookies := req.Cookies()
+ for _, newCookie := range s.Data.Cookies {
+ for i, existing := range existingCookies {
+ if existing.Name == newCookie.Key {
+ existingCookies = append(existingCookies[:i], existingCookies[i+1:]...)
+ break
+ }
+ }
+ }
+ // Clear and reset remaining cookies
+ req.Header.Del("Cookie")
+ for _, cookie := range existingCookies {
+ req.AddCookie(cookie)
+ }Committable suggestion skipped: line range outside the PR's diff.
| func Test_NewStatsDatabase(t *testing.T) { | ||
| db, err := NewSqliteStatsDatabase("test") | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test isolation and cleanup
The test should ensure proper cleanup before and after execution to maintain isolation:
- The database name "test" is too generic and could conflict with other tests
- Missing cleanup of any existing database file before the test starts
Consider this improvement:
func Test_NewStatsDatabase(t *testing.T) {
+ dbFile := "test.stats.db"
+ // Cleanup before test
+ os.Remove(dbFile)
+
- db, err := NewSqliteStatsDatabase("test")
+ db, err := NewSqliteStatsDatabase(dbFile)
require.NoError(t, err)
+
+ // Ensure cleanup after test
+ t.Cleanup(func() {
+ db.Close()
+ os.Remove(dbFile)
+ })📝 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.
| func Test_NewStatsDatabase(t *testing.T) { | |
| db, err := NewSqliteStatsDatabase("test") | |
| require.NoError(t, err) | |
| func Test_NewStatsDatabase(t *testing.T) { | |
| dbFile := "test.stats.db" | |
| // Cleanup before test | |
| os.Remove(dbFile) | |
| db, err := NewSqliteStatsDatabase(dbFile) | |
| require.NoError(t, err) | |
| // Ensure cleanup after test | |
| t.Cleanup(func() { | |
| db.Close() | |
| os.Remove(dbFile) | |
| }) |
| var siteName string | ||
| err = db.db.QueryRow("SELECT template_name FROM templates WHERE template_id = 1").Scan(&siteName) | ||
| require.NoError(t, err) | ||
| require.Equal(t, "apache-struts2-001", siteName) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct SQL queries and hardcoded IDs in tests
The verification has two issues:
- It bypasses the database interface by using direct SQL
- Assumes template_id = 1, which is fragile
Consider this improvement:
- var siteName string
- err = db.db.QueryRow("SELECT template_name FROM templates WHERE template_id = 1").Scan(&siteName)
- require.NoError(t, err)
- require.Equal(t, "apache-struts2-001", siteName)
+ // Add method to StatsDatabase interface
+ templateName, err := db.GetTemplateNameByID("apache-struts2-001")
+ require.NoError(t, err)
+ require.Equal(t, "apache-struts2-001", templateName)Committable suggestion skipped: line range outside the PR's diff.
| err := tx.QueryRow( | ||
| `INSERT OR IGNORE INTO sites (site_name) | ||
| VALUES (?) RETURNING site_id | ||
| `, siteName).Scan(&siteID) | ||
| if err != nil { | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
Correct the usage of INSERT OR IGNORE ... RETURNING to reliably retrieve IDs
The methods getSiteID, getTemplateID, and getComponentID use INSERT OR IGNORE INTO ... RETURNING .... In SQLite, if the INSERT is ignored due to a conflict (e.g., duplicate key), the RETURNING clause does not return any rows, resulting in sql.ErrNoRows. This means existing IDs are not retrieved, which can cause the methods to fail.
Modify the queries to ensure that the ID is retrieved whether the insert occurs or not. One approach is to SELECT the ID after attempting the INSERT. Here's how you can modify getSiteID:
func (s *sqliteStatsDatabase) getSiteID(tx *sql.Tx, siteName string) (int, error) {
var siteID int
s.cacheMutex.Lock()
if id, ok := s.siteIDCache[siteName]; ok {
s.cacheMutex.Unlock()
return id, nil
}
s.cacheMutex.Unlock()
- err := tx.QueryRow(
- `INSERT OR IGNORE INTO sites (site_name)
- VALUES (?) RETURNING site_id
- `, siteName).Scan(&siteID)
+ _, err := tx.Exec(
+ `INSERT OR IGNORE INTO sites (site_name)
+ VALUES (?)
+ `, siteName)
+ if err != nil {
+ return 0, err
+ }
+ err = tx.QueryRow(
+ `SELECT site_id FROM sites WHERE site_name = ?`,
+ siteName).Scan(&siteID)
if err != nil {
return 0, err
}
// Cache the site_id
s.cacheMutex.Lock()
s.siteIDCache[siteName] = siteID
s.cacheMutex.Unlock()
return siteID, nil
}Apply similar changes to getTemplateID and getComponentID to ensure they reliably retrieve the IDs.
Also applies to: 187-193, 212-219
| if err != nil { | ||
| return nil, errors.Wrap(err, "Could not create loadeopts.") | ||
| } |
There was a problem hiding this comment.
Correct the typo in the error message
The error message "Could not create loadeopts." contains a typo. It should likely be "Could not create loader options." Please correct it for clarity.
Apply this diff to fix the typo:
- return nil, errors.Wrap(err, "Could not create loadeopts.")
+ return nil, errors.Wrap(err, "Could not create loader options.")📝 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.
| if err != nil { | |
| return nil, errors.Wrap(err, "Could not create loadeopts.") | |
| } | |
| if err != nil { | |
| return nil, errors.Wrap(err, "Could not create loader options.") | |
| } |
| } `json:"request"` | ||
| } | ||
|
|
||
| func (n *nucleiExecutor) ExecuteScan(target PostReuestsHandlerRequest) error { |
There was a problem hiding this comment.
Fix the typo in the function parameter type
The type PostReuestsHandlerRequest in the function signature seems to have a typo in "Reuests." It should likely be PostRequestsHandlerRequest.
Apply this diff to correct the type name:
-func (n *nucleiExecutor) ExecuteScan(target PostReuestsHandlerRequest) error {
+func (n *nucleiExecutor) ExecuteScan(target PostRequestsHandlerRequest) 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.
| func (n *nucleiExecutor) ExecuteScan(target PostReuestsHandlerRequest) error { | |
| func (n *nucleiExecutor) ExecuteScan(target PostRequestsHandlerRequest) error { |
| if err != nil { | ||
| return errors.Wrap(err, "could not create input provider") | ||
| } | ||
| _ = n.engine.ExecuteScanWithOpts(context.Background(), finalTemplates, inputProvider, true) |
There was a problem hiding this comment.
Handle the error returned by ExecuteScanWithOpts
The error returned by n.engine.ExecuteScanWithOpts is currently ignored. To ensure that any issues during execution are not missed, capture and handle the error appropriately.
Apply this diff to handle the error:
- _ = n.engine.ExecuteScanWithOpts(context.Background(), finalTemplates, inputProvider, true)
+ if err := n.engine.ExecuteScanWithOpts(context.Background(), finalTemplates, inputProvider, true); err != nil {
+ return errors.Wrap(err, "could not execute scan with options")
+ }Committable suggestion skipped: line range outside the PR's diff.
| @@ -215,6 +221,10 @@ | |||
| } | |||
| } | |||
|
|
|||
| var ( | |||
| nameGenerator = namegenerator.NewNameGenerator(time.Now().UnixNano()) | |||
There was a problem hiding this comment.
Remove unused variable nameGenerator
The variable nameGenerator is declared but not used, causing a linting error. Please remove it to clean up the code and resolve the lint warning.
Apply this diff to remove the unused variable:
-var (
- nameGenerator = namegenerator.NewNameGenerator(time.Now().UnixNano())
-)📝 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.
| nameGenerator = namegenerator.NewNameGenerator(time.Now().UnixNano()) |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 225-225:
var nameGenerator is unused (unused)
🪛 golangci-lint (1.62.2)
225-225: var nameGenerator is unused
(unused)
Proposed changes
Added initial DAST live server implementation
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests