feat(openapi/swagger): direct fuzzing using target url #6542
Conversation
WalkthroughCreate a single runner-owned temp directory and pass it to input providers; add a SpecDownloader interface (accepting an optional retryablehttp client) plus OpenAPI and Swagger downloaders that fetch, validate/augment, and persist remote specs into that temp dir; integrate spec download into provider flow for openapi/swagger modes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as Runner
participant IP as InputProvider
participant D as SpecDownloader
participant S as RemoteServer
participant FS as FileSystem
R->>R: create tmpDir (nuclei-tmp-*)
R->>IP: New(options with TempDir=tmpDir)
IP->>IP: determine input mode
alt mode is openapi or swagger
IP->>IP: require single target URL
IP->>D: Download(url, tmpDir, httpClient?)
D->>S: HTTP GET spec (uses provided retryablehttp.Client or fallback)
S-->>D: spec (JSON/YAML) or error
D->>D: validate spec version/format
D->>D: inject servers/host/schemes if missing
D->>FS: write file under tmpDir/{openapi,swagger}/
FS-->>D: filepath
D-->>IP: filepath
IP->>IP: set TargetsFilePath = filepath
end
IP->>IP: delegate to HTTP input provider using TargetsFilePath
IP-->>R: provider initialized
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
pkg/input/formats/formats.go (1)
50-57: SpecDownloader interface: LGTM.Clear contract; fits the provider flow. Consider documenting size/time limits expectations next to Download for uniform impls.
pkg/input/provider/interface.go (1)
114-144: Support local file targets and guard empty TempDir.
- If the single “target” is a local path (no http/https), we should treat it as InputFile to preserve existing CLI behavior expectations.
- If TempDir is empty (e.g., runner failed to set it), downloaders will write into cwd (“openapi”/“swagger” folders) and Close won’t clean them. Add a small fallback.
Apply:
- target := opts.Options.Targets[0] - if strings.HasPrefix(target, "http://") || strings.HasPrefix(target, "https://") { + target := opts.Options.Targets[0] + // local file path support + if !(strings.HasPrefix(target, "http://") || strings.HasPrefix(target, "https://")) { + opts.Options.TargetsFilePath = target + } else { + // downloader path; ensure TempDir is set + if opts.TempDir == "" { + if tmp, err := os.MkdirTemp("", "nuclei-tmp-*"); err == nil { + opts.TempDir = tmp + // note: provider.Close() does not own this dir; runner remains the owner + } + } var downloader formats.SpecDownloader var tempFile string var err error @@ opts.Options.TargetsFilePath = tempFile - } + }Import “os” at top if not already in this file.
Please verify we don’t double-cleanup TempDir (provider must not remove it; runner owns it).
pkg/input/formats/openapi/downloader_test.go (2)
13-27: Avoid 35s sleeps; run tests in parallel; override client timeout.Tests currently sleep 35s to trigger timeouts, slowing CI. Make httpTimeout overridable and use short sleeps; also mark tests parallel where safe.
Apply:
@@ func TestOpenAPIDownloader_SupportedExtensions(t *testing.T) { + t.Parallel() @@ func TestOpenAPIDownloader_Download_Success(t *testing.T) { + t.Parallel() @@ func TestOpenAPIDownloader_Download_NonJSONURL(t *testing.T) { + t.Parallel() @@ func TestOpenAPIDownloader_Download_HTTPError(t *testing.T) { + t.Parallel() @@ func TestOpenAPIDownloader_Download_InvalidJSON(t *testing.T) { + t.Parallel() @@ func TestOpenAPIDownloader_Download_Timeout(t *testing.T) { + t.Parallel() @@ - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(35 * time.Second) // Longer than 30 second timeout + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(500 * time.Millisecond) json.NewEncoder(w).Encode(map[string]interface{}{"test": "data"}) })) @@ - downloader := &OpenAPIDownloader{} + downloader := &OpenAPIDownloader{} + orig := httpTimeout + httpTimeout = 100 * time.Millisecond + defer func() { httpTimeout = orig }() @@ func TestOpenAPIDownloader_Download_WithExistingServers(t *testing.T) { + t.Parallel()This drops test runtime dramatically and keeps coverage intact.
Also applies to: 29-96, 98-115, 116-135, 136-156, 157-176, 178-235, 236-240
29-96: Nit: also assert default server URL uses the target host.Small assertion improvement to catch regressions.
Example:
servers := downloadedSpec["servers"].([]interface{}) u := servers[0].(map[string]interface{})["url"].(string) if !strings.Contains(u, "://"+strings.TrimPrefix(strings.TrimPrefix(server.URL, "http://"), "https://")) { t.Errorf("unexpected server URL: %s", u) }pkg/input/formats/swagger/downloader_test.go (2)
15-29: Speed up tests and parallelize; override timeout.Reduce timeout sleep and override timeout var; mark tests parallel.
Apply:
@@ func TestSwaggerDownloader_SupportedExtensions(t *testing.T) { + t.Parallel() @@ func TestSwaggerDownloader_Download_JSON_Success(t *testing.T) { + t.Parallel() @@ func TestSwaggerDownloader_Download_YAML_Success(t *testing.T) { + t.Parallel() @@ func TestSwaggerDownloader_Download_UnsupportedExtension(t *testing.T) { + t.Parallel() @@ func TestSwaggerDownloader_Download_HTTPError(t *testing.T) { + t.Parallel() @@ func TestSwaggerDownloader_Download_InvalidJSON(t *testing.T) { + t.Parallel() @@ func TestSwaggerDownloader_Download_InvalidYAML(t *testing.T) { + t.Parallel() @@ func TestSwaggerDownloader_Download_Timeout(t *testing.T) { + t.Parallel() @@ - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(35 * time.Second) // Longer than 30 second timeout + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(500 * time.Millisecond) json.NewEncoder(w).Encode(map[string]interface{}{"test": "data"}) })) @@ - downloader := &SwaggerDownloader{} + downloader := &SwaggerDownloader{} + // if you expose httpTimeout as a package var, override it here similarly to openapi tests @@ func TestSwaggerDownloader_Download_WithExistingHost(t *testing.T) { + t.Parallel()If you promote swagger’s timeout to a package var, also override it to ~100ms in the timeout test.
Also applies to: 31-91, 92-147, 149-166, 167-185, 187-206, 208-227, 229-248, 250-300, 302-306
85-91: Nit: verify host value equals target host.Adds a stronger check beyond presence.
Example:
if downloadedSpec["host"] != strings.TrimPrefix(strings.TrimPrefix(server.URL, "http://"), "https://") { t.Errorf("unexpected host: %v", downloadedSpec["host"]) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/runner/runner.go(1 hunks)pkg/input/formats/formats.go(1 hunks)pkg/input/formats/openapi/downloader.go(1 hunks)pkg/input/formats/openapi/downloader_test.go(1 hunks)pkg/input/formats/swagger/downloader.go(1 hunks)pkg/input/formats/swagger/downloader_test.go(1 hunks)pkg/input/provider/interface.go(3 hunks)
�� Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/input/formats/openapi/downloader.gopkg/input/formats/formats.gopkg/input/formats/openapi/downloader_test.gointernal/runner/runner.gopkg/input/provider/interface.gopkg/input/formats/swagger/downloader.gopkg/input/formats/swagger/downloader_test.go
🧬 Code graph analysis (6)
pkg/input/formats/openapi/downloader.go (1)
pkg/input/formats/formats.go (1)
SpecDownloader(51-57)
pkg/input/formats/openapi/downloader_test.go (1)
pkg/input/formats/openapi/downloader.go (1)
OpenAPIDownloader(19-19)
internal/runner/runner.go (1)
pkg/input/provider/interface.go (2)
NewInputProvider(88-157)InputOptions(76-84)
pkg/input/provider/interface.go (6)
pkg/types/types.go (1)
Options(34-472)pkg/input/formats/formats.go (2)
SpecDownloader(51-57)InputFormatOptions(20-37)pkg/input/formats/openapi/downloader.go (1)
NewDownloader(22-24)pkg/input/formats/swagger/downloader.go (1)
NewDownloader(23-25)pkg/input/provider/http/multiformat.go (2)
NewHttpInputProvider(48-96)HttpMultiFormatOptions(22-32)pkg/protocols/common/generators/maps.go (1)
MergeMaps(48-54)
pkg/input/formats/swagger/downloader.go (2)
pkg/input/formats/openapi/downloader.go (1)
NewDownloader(22-24)pkg/input/formats/formats.go (1)
SpecDownloader(51-57)
pkg/input/formats/swagger/downloader_test.go (1)
pkg/input/formats/swagger/downloader.go (1)
SwaggerDownloader(20-20)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/runner/runner.go (1)
257-271: TmpDir lifecycle fix looks solid.Mandatory MkdirTemp + deferred cleanup on constructor failure, and disabling on success, prevents temp dir leaks. Nice resolution of the earlier concern.
Also applies to: 404-406
🧹 Nitpick comments (7)
pkg/input/formats/swagger/downloader.go (3)
51-55: Avoid wrapping and discarding close errors in defers.Wrapping without using/logging the error is a no‑op and may trip linters.
Apply:
- defer func() { - if err := resp.Body.Close(); err != nil { - errors.Wrap(err, "failed to close response body") - } - }() + defer func() { _ = resp.Body.Close() }() @@ - defer func() { - if err := file.Close(); err != nil { - errors.Wrap(err, "failed to close file") - } - }() + defer func() { _ = file.Close() }()Also applies to: 143-147
42-45: Make timeout/limit package-level vars for testability and consistency.Define httpTimeout and maxSpecSizeBytes at package scope (overridable by tests), and reuse across handlers.
Apply:
package swagger @@ -import ( +import ( @@ "time" @@ ) +// httpTimeout is overridden in tests when needed +var httpTimeout = 30 * time.Second +const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB + @@ - var httpTimeout = 30 * time.Second - const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB client := &http.Client{Timeout: httpTimeout}
121-133: Optional: use os.CreateTemp for unique filenames.Avoid same‑second collisions and simplify naming.
Apply:
- var filename string - var content []byte + var filename string + var content []byte @@ - if isYAML { - filename = fmt.Sprintf("swagger-spec-%d.yaml", time.Now().Unix()) + if isYAML { + filename = "swagger-spec-*.yaml" content, err = yaml.Marshal(spec) @@ - } else { - filename = fmt.Sprintf("swagger-spec-%d.json", time.Now().Unix()) + } else { + filename = "swagger-spec-*.json" content, err = json.Marshal(spec) @@ - filePath := filepath.Join(swaggerDir, filename) + tmpf, err := os.CreateTemp(swaggerDir, filename) + if err != nil { + return "", errors.Wrap(err, "failed to create temp file") + } + filePath := tmpf.Name() + file := tmpfpkg/input/formats/openapi/downloader_test.go (2)
181-207: Speed up timeout test; avoid 35s sleep.Expose downloader timeout as a package var and override it in the test to keep CI fast and stable.
After introducing a package‑level
httpTimeoutin openapi/downloader.go, change the test to:func TestOpenAPIDownloader_Download_Timeout(t *testing.T) { - // Create mock server with delay + // Speed up by shortening package timeout just for this test + orig := httpTimeout + httpTimeout = 100 * time.Millisecond + t.Cleanup(func() { httpTimeout = orig }) + + // Create mock server with delay > timeout server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(35 * time.Second) // Longer than 30 second timeout + time.Sleep(httpTimeout + 50*time.Millisecond) if err := json.NewEncoder(w).Encode(map[string]interface{}{"test": "data"}); err != nil { http.Error(w, "failed to encode response", http.StatusInternalServerError) } }))
68-69: Minor: prefer constructor to instantiate downloader.Keeps tests aligned with public API.
Apply (example in one place; replicate in others):
- downloader := &OpenAPIDownloader{} + downloader := NewDownloader()Also applies to: 117-118, 146-147, 174-176, 245-247
pkg/input/formats/openapi/downloader.go (2)
26-36: Broaden format support and improve testability.Support YAML in addition to JSON, and hoist timeout/limit to package scope. Clean up defers and extend SupportedExtensions.
Apply:
import ( "encoding/json" "fmt" "io" "net/http" "net/url" "os" "path/filepath" "strings" "time" "github.com/pkg/errors" "github.com/projectdiscovery/nuclei/v3/pkg/input/formats" + "gopkg.in/yaml.v3" ) +// httpTimeout can be overridden by tests +var httpTimeout = 30 * time.Second +const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB + @@ - // Validate URL format, OpenAPI 3.0 specs are typically JSON - if !strings.HasSuffix(urlStr, ".json") && !strings.Contains(urlStr, "openapi") { - return "", fmt.Errorf("URL does not appear to be an OpenAPI JSON spec") - } - - var httpTimeout = 30 * time.Second - const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB + // Light validation; rely primarily on content + if !(strings.HasSuffix(urlStr, ".json") || strings.HasSuffix(urlStr, ".yaml") || strings.HasSuffix(urlStr, ".yml") || strings.Contains(urlStr, "openapi")) { + return "", fmt.Errorf("URL does not appear to be an OpenAPI spec") + } client := &http.Client{Timeout: httpTimeout} @@ - var spec map[string]interface{} - if err := json.Unmarshal(bodyBytes, &spec); err != nil { - return "", fmt.Errorf("downloaded content is not valid JSON: %w", err) - } + var spec map[string]interface{} + // Try JSON first; fallback to YAML + if err := json.Unmarshal(bodyBytes, &spec); err != nil { + if yerr := yaml.Unmarshal(bodyBytes, &spec); yerr != nil { + return "", fmt.Errorf("downloaded content is neither valid JSON nor YAML: %w", yerr) + } + } @@ - if err := os.MkdirAll(openapiDir, 0755); err != nil { - - return "", errors.Wrap(err, "failed to create openapi directory") + if err := os.MkdirAll(openapiDir, 0755); err != nil { + return "", errors.Wrap(err, "failed to create openapi directory") } @@ func (d *OpenAPIDownloader) SupportedExtensions() []string { - return []string{".json"} + return []string{".json", ".yaml", ".yml"} }Also applies to: 57-66, 93-101, 131-134
42-46: Avoid wrapping and discarding close errors.Same pattern as Swagger: drop Wrap on defers and discard the error.
Apply:
- defer func() { - if err := resp.Body.Close(); err != nil { - errors.Wrap(err, "failed to close response body") - } - }() + defer func() { _ = resp.Body.Close() }() @@ - defer func() { - if err := file.Close(); err != nil { - errors.Wrap(err, "failed to close file") - } - }() + defer func() { _ = file.Close() }()Also applies to: 116-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/runner/runner.go(2 hunks)pkg/input/formats/openapi/downloader.go(1 hunks)pkg/input/formats/openapi/downloader_test.go(1 hunks)pkg/input/formats/swagger/downloader.go(1 hunks)pkg/input/formats/swagger/downloader_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/input/formats/swagger/downloader_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/input/formats/openapi/downloader_test.gointernal/runner/runner.gopkg/input/formats/swagger/downloader.gopkg/input/formats/openapi/downloader.go
🧬 Code graph analysis (4)
pkg/input/formats/openapi/downloader_test.go (1)
pkg/input/formats/openapi/downloader.go (1)
OpenAPIDownloader(19-19)
internal/runner/runner.go (1)
pkg/input/provider/interface.go (2)
NewInputProvider(88-157)InputOptions(76-84)
pkg/input/formats/swagger/downloader.go (2)
pkg/input/formats/openapi/downloader.go (1)
NewDownloader(22-24)pkg/input/formats/formats.go (1)
SpecDownloader(51-57)
pkg/input/formats/openapi/downloader.go (2)
pkg/input/formats/swagger/downloader.go (1)
NewDownloader(23-25)pkg/input/formats/formats.go (1)
SpecDownloader(51-57)
🔇 Additional comments (1)
internal/runner/runner.go (1)
273-273: Good: TempDir plumbed into input provider.This unblocks direct OpenAPI/Swagger URL downloading. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pkg/input/formats/openapi/downloader.go (1)
33-33: Timeout should be package-level variable for test configurability.The past review feedback requested a package-level
httpTimeoutvariable to allow tests to override it, but this implementation uses a local variable. This prevents tests from reducing the timeout, leading to slower test execution.Apply this diff to implement the intended design:
+// httpTimeout can be overridden in tests +var httpTimeout = 30 * time.Second +const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB + // This function downloads an OpenAPI 3.0 spec from the given URL and saves it to tmpDir func (d *OpenAPIDownloader) Download(urlStr, tmpDir string) (string, error) { // Validate URL format, OpenAPI 3.0 specs are typically JSON if !strings.HasSuffix(urlStr, ".json") && !strings.Contains(urlStr, "openapi") { return "", fmt.Errorf("URL does not appear to be an OpenAPI JSON spec") } - var httpTimeout = 30 * time.Second - const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB client := &http.Client{Timeout: httpTimeout}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/input/formats/openapi/downloader.go(1 hunks)pkg/input/formats/swagger/downloader.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/input/formats/swagger/downloader.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/input/formats/openapi/downloader.go
🧬 Code graph analysis (1)
pkg/input/formats/openapi/downloader.go (2)
pkg/input/formats/swagger/downloader.go (1)
NewDownloader(23-25)pkg/input/formats/formats.go (1)
SpecDownloader(51-57)
🔇 Additional comments (8)
pkg/input/formats/openapi/downloader.go (8)
1-16: LGTM!Package declaration and imports are appropriate for the downloader functionality.
18-24: LGTM!Clean constructor pattern consistent with the codebase.
52-55: LGTM!Proper use of
io.LimitReaderto bound response size, addressing the past review feedback.
57-61: LGTM!JSON validation is properly implemented with clear error messages.
63-72: LGTM!OpenAPI version validation is thorough with clear error messages.
74-91: LGTM!Server configuration logic correctly derives the scheme from the URL with a sensible fallback, addressing the past review feedback.
122-125: LGTM!Good cleanup pattern with
os.Removeon write failure.
130-133: LGTM!Straightforward implementation of the interface method.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/input/formats/openapi/downloader.go(1 hunks)pkg/input/formats/swagger/downloader.go(1 hunks)pkg/input/formats/swagger/downloader_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/input/formats/swagger/downloader_test.go
- pkg/input/formats/openapi/downloader.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/input/formats/swagger/downloader.go
🧬 Code graph analysis (1)
pkg/input/formats/swagger/downloader.go (2)
pkg/input/formats/openapi/downloader.go (1)
NewDownloader(22-24)pkg/input/formats/formats.go (1)
SpecDownloader(51-57)
🔇 Additional comments (5)
pkg/input/formats/swagger/downloader.go (5)
19-25: LGTM!The constructor follows Go best practices by returning the interface type, and the empty struct is appropriate for this stateless downloader.
42-86: LGTM!The download logic properly addresses security and reliability concerns with bounded reads (10MB limit), configurable timeout (30s), format detection (JSON-first with YAML fallback), and robust Swagger 2.0 validation.
88-108: LGTM!The URL parsing and spec augmentation logic correctly derives the host and scheme (with a sensible "https" default) and only adds missing fields, preserving any existing configuration in the original spec.
110-151: LGTM!The file writing logic is robust: it preserves the original format (JSON or YAML), creates the necessary directory structure, properly handles errors with cleanup on write failure, and correctly uses deferred close. The previous err-shadowing issue has been addressed.
154-157: LGTM!The method correctly returns the supported extensions list. Note: the duplication with the Download method's local list has been flagged separately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/input/formats/swagger/downloader.go (1)
42-57: LGTM!HTTP client setup properly includes timeout and bounded reading safeguards. Error handling and resource cleanup are correct.
Optional: Consider package-level constants for testability.
If tests need to override timeout or size limits, hoisting
httpTimeoutandmaxSpecSizeBytesto package level (as done in OpenAPI) would improve testability. This was mentioned as optional in prior reviews.+var httpTimeout = 30 * time.Second +const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB + // This function downloads a Swagger 2.0 spec from the given URL and saves it to tmpDir func (d *SwaggerDownloader) Download(urlStr, tmpDir string) (string, error) { // Swagger can be JSON or YAML supportedExts := d.SupportedExtensions() isSupported := false for _, ext := range supportedExts { if strings.HasSuffix(urlStr, ext) { isSupported = true break } } if !isSupported { return "", fmt.Errorf("URL does not appear to be a Swagger spec (supported: %v)", supportedExts) } - var httpTimeout = 30 * time.Second - const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB client := &http.Client{Timeout: httpTimeout}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/input/formats/swagger/downloader.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/input/formats/swagger/downloader.go
🧬 Code graph analysis (1)
pkg/input/formats/swagger/downloader.go (2)
pkg/input/formats/openapi/downloader.go (1)
NewDownloader(22-24)pkg/input/formats/formats.go (1)
SpecDownloader(51-57)
🔇 Additional comments (8)
pkg/input/formats/swagger/downloader.go (8)
1-17: LGTM!Package declaration and imports are appropriate. The use of
pkg/errorsfor error wrapping andyaml.v3for YAML parsing aligns with project conventions.
19-25: LGTM!Type declaration and constructor follow the established pattern from the OpenAPI downloader, ensuring consistency across format handlers.
28-40: LGTM!Extension validation correctly uses the
SupportedExtensions()method, eliminating code duplication. The error message helpfully lists supported formats.
59-75: LGTM!Bounded reading with
io.LimitReaderprotects against resource exhaustion. Format detection correctly tries JSON before YAML and preserves the format for later re-serialization.
77-86: LGTM!Swagger 2.0 version validation is correct and robust. The logic properly checks field existence, type, and version prefix, with helpful error messages for debugging.
88-98: LGTM!URL parsing and scheme derivation correctly default to "https" and handle parse errors appropriately. The scheme derivation is properly positioned for reuse.
100-149: LGTM!Spec augmentation correctly adds missing
hostandschemesfields without overwriting existing values. File writing properly avoids error shadowing by usingwriteErr, and cleanup is handled correctly.
154-157: LGTM!
SupportedExtensions()correctly returns the standard Swagger file extensions.
|
Hi @dogancanbakir , could you run the workflows please? thank you |
|
Hi @dogancanbakir , could you approve this and include it in the next release? do you have any concerns? thank you |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
pkg/input/formats/formats.go (1)
10-10: Avoid hard dependency + add cancellation support on SpecDownloader.Coupling this interface to retryablehttp-go at the formats layer leaks a concrete client type and makes cancellation impossible. Prefer passing a context and a standard http client (or a minimal Doer) to keep packages decoupled and support timeouts/cancellation end-to-end.
Example shape:
-import "github.com/projectdiscovery/retryablehttp-go" +import "context" +import "net/http" type SpecDownloader interface { - Download(url, tmpDir string, httpClient *retryablehttp.Client) (string, error) + Download(ctx context.Context, url, tmpDir string, httpClient *http.Client) (string, error) SupportedExtensions() []string }Follow-up: wire ctx from runner/provider. As per coding guidelines.
Also applies to: 51-59
pkg/input/formats/swagger/downloader_test.go (2)
268-294: Timeout test is 30–35s; inject a short-timeout client to keep tests fast.Build a retryable client with a small timeout and reduce server sleep. This keeps CI fast and reliable.
@@ - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(35 * time.Second) // Longer than 30 second timeout + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(1 * time.Second) @@ tmpDir, err := os.MkdirTemp("", "swagger_test") @@ - downloader := &SwaggerDownloader{} - _, err = downloader.Download(server.URL+"/swagger.json", tmpDir, nil) + downloader := &SwaggerDownloader{} + httpClient := retryablehttp.NewClient() + httpClient.RetryMax = 0 + httpClient.HTTPClient.Timeout = 200 * time.Millisecond + _, err = downloader.Download(server.URL+"/swagger.json", tmpDir, httpClient) if err == nil { t.Error("Expected timeout error, but got none") }Note: add
github.com/projectdiscovery/retryablehttp-goimport at top. As per coding guidelines.
15-29: Avoid order-sensitive extension assertions.Assert by set membership to prevent brittle failures if order changes.
- for i, ext := range extensions { - if ext != expected[i] { - t.Errorf("Expected extension %s, got %s", expected[i], ext) - } - } + got := map[string]struct{}{} + for _, ext := range extensions { got[ext] = struct{}{} } + for _, want := range expected { + if _, ok := got[want]; !ok { + t.Errorf("missing extension %s", want) + } + }pkg/input/formats/openapi/downloader.go (1)
30-33: URL suffix restriction may reject valid OpenAPI endpoints.Requiring
.jsononly blocks common endpoints like/v3/api-docs. Consider allowing non-.jsonURLs and validating by content (JSON parse + openapi: "3.*"), or at least checkContent-Type: application/json.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/input/formats/formats.go(2 hunks)pkg/input/formats/openapi/downloader.go(1 hunks)pkg/input/formats/openapi/downloader_test.go(1 hunks)pkg/input/formats/swagger/downloader.go(1 hunks)pkg/input/formats/swagger/downloader_test.go(1 hunks)pkg/input/provider/interface.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/input/formats/openapi/downloader_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/input/formats/swagger/downloader_test.gopkg/input/formats/formats.gopkg/input/provider/interface.gopkg/input/formats/swagger/downloader.gopkg/input/formats/openapi/downloader.go
🧬 Code graph analysis (4)
pkg/input/formats/swagger/downloader_test.go (1)
pkg/input/formats/swagger/downloader.go (1)
SwaggerDownloader(22-22)
pkg/input/provider/interface.go (6)
pkg/types/types.go (1)
Options(34-472)pkg/input/formats/formats.go (2)
SpecDownloader(52-59)InputFormatOptions(21-38)pkg/protocols/common/protocolstate/state.go (1)
GetDialersWithId(39-45)pkg/input/formats/openapi/downloader.go (1)
NewDownloader(24-26)pkg/input/formats/swagger/downloader.go (1)
NewDownloader(25-27)pkg/input/provider/http/multiformat.go (2)
NewHttpInputProvider(48-96)HttpMultiFormatOptions(22-32)
pkg/input/formats/swagger/downloader.go (2)
pkg/input/formats/openapi/downloader.go (1)
NewDownloader(24-26)pkg/input/formats/formats.go (1)
SpecDownloader(52-59)
pkg/input/formats/openapi/downloader.go (2)
pkg/input/formats/swagger/downloader.go (1)
NewDownloader(25-27)pkg/input/formats/formats.go (1)
SpecDownloader(52-59)
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pkg/input/formats/openapi/downloader.go (2)
34-34: Make timeout and size limit configurable for testing.Past review comments suggest exposing
maxSpecSizeBytesas a package-level constant and the timeout as a package variable so tests can override them.Apply this diff:
+const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB +var httpTimeout = 30 * time.Second + // OpenAPIDownloader implements the SpecDownloader interface for OpenAPI 3.0 specs type OpenAPIDownloader struct{}Then update line 42 to use
httpTimeout:} else { // Fallback to simple client if no httpClient provided - client = &http.Client{Timeout: 30 * time.Second} + client = &http.Client{Timeout: httpTimeout} }And remove the const declaration from inside the function.
Based on past review comments.
36-56: Bypass retryable client's retry logic by extracting HTTPClient.Extracting
httpClient.HTTPClientand callingclient.Get()defeats the purpose of passing aretryablehttp.Client. This bypasses retry logic, backoff, and other resilience features.Apply this diff to use the retryable client properly:
const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB - // Use provided httpClient or create a fallback - var client *http.Client - if httpClient != nil { - client = httpClient.HTTPClient - } else { - // Fallback to simple client if no httpClient provided - client = &http.Client{Timeout: 30 * time.Second} - } - - resp, err := client.Get(urlStr) + // Use provided httpClient or create a fallback + var resp *http.Response + var err error + if httpClient != nil { + req, reqErr := retryablehttp.NewRequest("GET", urlStr, nil) + if reqErr != nil { + return "", errors.Wrap(reqErr, "failed to create request") + } + resp, err = httpClient.Do(req) + } else { + // Fallback to simple client if no httpClient provided + stdClient := &http.Client{Timeout: 30 * time.Second} + resp, err = stdClient.Get(urlStr) + } if err != nil { return "", errors.Wrap(err, "failed to download OpenAPI spec") }Based on past review comments.
pkg/input/formats/swagger/downloader.go (1)
43-57: Bypass retryable client's retry logic by extracting HTTPClient.Extracting
httpClient.HTTPClientand callingclient.Get()defeats the purpose of passing aretryablehttp.Client. This bypasses retry logic, backoff, and other resilience features.Apply this diff to use the retryable client properly:
const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB - // Use provided httpClient or create a fallback - var client *http.Client - if httpClient != nil { - client = httpClient.HTTPClient - } else { - // Fallback to simple client if no httpClient provided - client = &http.Client{Timeout: 30 * time.Second} - } - - resp, err := client.Get(urlStr) + // Use provided httpClient or create a fallback + var resp *http.Response + var err error + if httpClient != nil { + req, reqErr := retryablehttp.NewRequest("GET", urlStr, nil) + if reqErr != nil { + return "", errors.Wrap(reqErr, "failed to create request") + } + resp, err = httpClient.Do(req) + } else { + // Fallback to simple client if no httpClient provided + stdClient := &http.Client{Timeout: 30 * time.Second} + resp, err = stdClient.Get(urlStr) + } if err != nil { return "", errors.Wrap(err, "failed to download Swagger spec") }Based on past review comments.
🧹 Nitpick comments (1)
pkg/input/formats/swagger/downloader.go (1)
43-43: Make timeout and size limit configurable for testing.Past review comments suggest exposing
maxSpecSizeBytesas a package-level constant and the timeout as a package variable so tests can override them.Apply this diff:
+const maxSpecSizeBytes = 10 * 1024 * 1024 // 10MB +var httpTimeout = 30 * time.Second + // SwaggerDownloader implements the SpecDownloader interface for Swagger 2.0 specs type SwaggerDownloader struct{}Then update line 51 to use
httpTimeout:} else { // Fallback to simple client if no httpClient provided - client = &http.Client{Timeout: 30 * time.Second} + client = &http.Client{Timeout: httpTimeout} }And remove the const declaration from inside the function.
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/input/formats/openapi/downloader.go(1 hunks)pkg/input/formats/swagger/downloader.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/input/formats/swagger/downloader.gopkg/input/formats/openapi/downloader.go
🧬 Code graph analysis (2)
pkg/input/formats/swagger/downloader.go (2)
pkg/input/formats/openapi/downloader.go (1)
NewDownloader(23-25)pkg/input/formats/formats.go (1)
SpecDownloader(52-59)
pkg/input/formats/openapi/downloader.go (2)
pkg/input/formats/swagger/downloader.go (1)
NewDownloader(24-26)pkg/input/formats/formats.go (1)
SpecDownloader(52-59)
🔇 Additional comments (7)
pkg/input/formats/swagger/downloader.go (4)
20-26: LGTM!The struct definition and factory function follow the standard Go pattern and correctly implement the
formats.SpecDownloaderinterface.
28-41: LGTM!The extension validation correctly uses
SupportedExtensions()to avoid code duplication and provides clear error messaging.
59-116: LGTM!The response handling, parsing, validation, and spec augmentation are well-implemented:
- Bounded reads prevent memory exhaustion
- Proper fallback from JSON to YAML parsing
- Correct Swagger 2.0 version validation
- Appropriate host and scheme injection with sensible defaults
118-165: LGTM!The file writing logic is robust:
- Proper directory creation with appropriate permissions
- Format preservation (JSON or YAML based on input)
- Correct error handling that avoids variable shadowing (past issue was addressed)
- Cleanup of incomplete files on write failures
- Clear list of supported extensions
pkg/input/formats/openapi/downloader.go (3)
19-25: LGTM!The struct definition and factory function follow the standard Go pattern and correctly implement the
formats.SpecDownloaderinterface.
27-32: LGTM!The URL validation requiring
.jsonsuffix is intentionally restrictive to prevent accidentally downloading HTML documentation pages instead of the actual JSON spec. This design decision was discussed in past reviews.
50-136: LGTM!The remaining implementation is robust and well-structured:
- Proper defer cleanup (past issues with error handling were addressed)
- Bounded reads prevent memory exhaustion
- Correct OpenAPI 3.0 version validation
- Appropriate server injection with sensible defaults (scheme defaulting to https)
- Clean error handling with correct variable naming to avoid shadowing
- Cleanup of incomplete files on write failures
✅ Actions performedComments resolved. |
3 participants
Proposed changes
In this PR i added downloader for openapi and swagger format. With this changes, user can use openapi/swagger format directly for URL target (-u) (close #6443).
Before, this command not supported
nuclei -u https://target.com/swagger.json -im swagger -t nuclei-fuzzing-templates/After
nuclei -u https://target.com/swagger.json -im swagger -t nuclei-fuzzing-templates/The openapi/swagger spec are stored at tmpDir from runner class, to make this tmpDir accessible to interface, it need to be passed, so i added tmpDir to inputOptions struct.
But this changes has limitation because only support 1 URL target. I think to make this usable for more than 1 target, some changes need to be added into list of InputOption to support list of httpInputProvider.
Run unit test
go test ./pkg/input/formats/openapi -vgo test ./pkg/input/formats/swagger -vChecklist
Summary by CodeRabbit
New Features
Improvements
Tests