fix(sdk): configure tmpDir for SDK#6596
Conversation
WalkthroughEngine now creates and tracks an engine-managed temporary directory (tmpDir), wires that path into executor options, exposes a new SDK option to set the temporary directory parent, and removes the temporary directory during engine shutdown. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Engine as NucleiEngine
participant OS
participant Exec as Executor
User->>Engine: NewNucleiEngine()
activate Engine
Engine->>OS: MkdirTemp(parentDir or system temp, "nuclei-tmp-*")
OS-->>Engine: tmpDir path
Engine->>Engine: set e.tmpDir
deactivate Engine
User->>Engine: ExecuteWithCallback()
activate Engine
Engine->>Exec: Init(... TemporaryDirectory = e.tmpDir ...)
Exec->>Exec: write templates into TemporaryDirectory
deactivate Engine
User->>Engine: Close()
activate Engine
Engine->>OS: RemoveAll(e.tmpDir)
OS-->>Engine: removed
deactivate Engine
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/sdk.go (1)
97-98: Consider clarifying the comment to reflect the specific purpose.The comment describes this as a "General purpose temporary directory," but based on the PR objectives and linked issue #6595, this directory is specifically intended for template files created by the SDK. Consider updating the comment to more accurately reflect this purpose.
Apply this diff to clarify the comment:
- // General purpose temporary directory + // Temporary directory for SDK-managed template files tmpDir string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/sdk.go(3 hunks)lib/sdk_private.go(2 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:
lib/sdk.golib/sdk_private.go
🧬 Code graph analysis (1)
lib/sdk_private.go (4)
pkg/protocols/protocols.go (1)
ExecutorOptions(61-141)pkg/progress/progress.go (1)
Progress(17-35)pkg/protocols/headless/engine/engine.go (1)
Browser(22-34)pkg/input/transform.go (1)
NewHelper(22-25)
🔇 Additional comments (5)
lib/sdk.go (2)
8-8: LGTM!The
osimport is correctly added to support the new temporary directory cleanup functionality.
238-240: Verify cleanup failure handling is acceptable for the use case.The error from
os.RemoveAllis silently ignored, which could leave temporary files on disk if removal fails (e.g., due to permissions or files still in use). While this follows the existing error-handling pattern incloseInternal, it may not fully address the disk exhaustion concern mentioned in issue #6595 if cleanup consistently fails.Consider whether it would be beneficial to log cleanup failures for debugging purposes.
If you'd like to add error logging, apply this diff:
if e.tmpDir != "" { - _ = os.RemoveAll(e.tmpDir) + if err := os.RemoveAll(e.tmpDir); err != nil { + e.Logger.Warningf("Failed to remove temporary directory %s: %v", e.tmpDir, err) + } }lib/sdk_private.go (3)
6-6: LGTM!The
osimport is correctly added to support temporary directory creation during initialization.
178-193: TemporaryDirectory field is properly used by the executor; address potential error handling gap.The
TemporaryDirectoryfield is actively used inpkg/protocols/code/code.go(lines 123 and 174) where it's passed togozero.NewSourceWithString()for code execution. The field is correctly initialized and propagated through the executor chain.However, there's a minor error handling concern: in
lib/sdk_private.go(lines 174–176), ifos.MkdirTemp()fails,e.tmpDirsilently remains empty string and gets passed downstream. Consider adding a fallback or logging to handle this case gracefully.
174-176: Silent tmpDir creation failure pattern confirmed but impact remains inconclusive.The code does exhibit the silent failure described: if
os.MkdirTempfails at line 174,e.tmpDirremains an empty string and initialization proceeds without error. This empty value is then passed toTemporaryDirectoryin ExecutorOptions (line 191) and forwarded togozero.NewSourceWithString()downstream without validation.However, verification reveals:
- No issue #6595 context or comments explaining this design choice
- The same pattern appears in
internal/runner/runner.go:389-390, suggesting consistency rather than oversight- The actual impact depends on how the external
gozeropackage handles an emptyTemporaryDirectorystring—whether it gracefully falls back or defaults to system/tmpThe concern is valid and warrants attention, but whether this pattern causes files to be created directly in
/tmp(and thus whether the silent failure is a bug or intentional graceful degradation) cannot be determined from the codebase alone.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/sdk.go (1)
238-240: LGTM! Cleanup logic correctly implemented.The temporary directory cleanup is properly placed in
closeInternal()with a defensive check for empty string. Usingos.RemoveAllensures all SDK-managed template files are removed on engine shutdown, which resolves the disk exhaustion issue described in #6595.The error from
os.RemoveAllis silently ignored, which is a common pattern in cleanup code. If enhanced observability is desired, you could optionally log any removal failures using the engine's Logger instance.Optional enhancement:
if e.tmpDir != "" { - _ = os.RemoveAll(e.tmpDir) + if err := os.RemoveAll(e.tmpDir); err != nil && e.Logger != nil { + e.Logger.Warning().Msgf("Failed to remove temporary directory %s: %v", e.tmpDir, err) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/sdk.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:
lib/sdk.go
🔇 Additional comments (2)
lib/sdk.go (2)
8-8: LGTM! Import added for cleanup functionality.The
osimport is correctly added to support the temporary directory removal in the cleanup logic.
97-98: LGTM! Field addition properly documented.The
tmpDirfield is appropriately unexported and clearly documented. This enables the SDK to track the temporary directory location for cleanup, directly addressing the issue described in #6595.
Mzack9999
left a comment
Mzack9999
left a comment
There was a problem hiding this comment.
Thanks for your PR! I would suggest to expose the temporary directory setting via SDK option, with something similar to:
// WithTemporaryDirectory allows setting a parent directory for SDK-managed temporary files.
// A temporary directory will be created inside the provided directory and cleaned up on engine close.
// If not set, a temporary directory will be automatically created in the system temp location.
// The parent directory will be created if it doesn't exist.
func WithTemporaryDirectory(parentDir string) NucleiSDKOptions {
return func(e *NucleiEngine) error {
e.tmpDir = parentDir
return nil
}
}
so the user can define a custom directory where to create the temporary directory. If not provide the default behavior would be the one implemented in this PR. What do you think?
|
Yep, I like that. I'll add user customizability soon, either tomorrow or Monday methinks. |
|
No worries! Thanks for your contribution, I tried to push directly the changes but looks like I haven't enough permissions on the fork. But it should suffice the snippet provided. Let me know if you need any support. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/config.go (1)
567-572: Consider adding input validation for the parentDir parameter.The function currently accepts any string value without validation. Consider adding basic validation to fail fast and provide clear error messages to users.
Apply this diff to add input validation:
func WithTemporaryDirectory(parentDir string) NucleiSDKOptions { return func(e *NucleiEngine) error { + if parentDir == "" { + return errors.New("parent directory path cannot be empty") + } e.tmpDir = parentDir return nil } }This ensures users get immediate feedback if they accidentally pass an empty string, rather than encountering cryptic errors later during engine initialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/config.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:
lib/config.go
🧬 Code graph analysis (1)
lib/config.go (1)
lib/sdk.go (2)
NucleiSDKOptions(37-37)NucleiEngine(59-99)
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 (1)
lib/sdk_private.go(2 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:
lib/sdk_private.go
🧬 Code graph analysis (1)
lib/sdk_private.go (2)
pkg/protocols/protocols.go (1)
ExecutorOptions(61-141)pkg/protocols/headless/engine/engine.go (1)
Browser(22-34)
🔇 Additional comments (2)
lib/sdk_private.go (2)
6-6: LGTM: Import addition is appropriate.The
osimport is necessary for theos.MkdirTempcall on line 175 and aligns with the temporary directory management feature.
182-197: LGTM: ExecutorOptions initialization properly updated.The refactoring to an explicit struct literal format improves readability, and the new
TemporaryDirectoryfield (line 195) is correctly wired toe.tmpDir. This enables the executor to use the SDK-managed temporary directory for template files, which aligns with the PR objectives.
|
@Mzack9999, the CodeRabbit bot has some valid feedback regarding documentation and implementation differences, and resource leak on init failure that I'd like to touch on. How do you want As for the resource leak on init failure, preferably when the Nuclei Engine fails to create there is no need to clean anything up (e.g. do not expect the caller to call |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/config.go (1)
568-580: Validate parentDir to prevent silent fallback to system temp directory.If
parentDiris an empty string,os.MkdirAll("", 0755)succeeds without creating anything, and thenos.MkdirTemp("", "nuclei-tmp-*")falls back to the system default temp location. This contradicts the user's intent to specify a custom parent directory and could lead to confusion when temporary files appear in an unexpected location.Apply this diff to add input validation:
func WithTemporaryDirectory(parentDir string) NucleiSDKOptions { return func(e *NucleiEngine) error { + if parentDir == "" { + return errors.New("parent directory path cannot be empty") + } if err := os.MkdirAll(parentDir, 0755); err != nil { return err }
🧹 Nitpick comments (1)
lib/config.go (1)
564-567: Consider clarifying the documentation for edge cases.The documentation could be more explicit about:
- What happens if
parentDiris an empty string (currently would fall back to system temp location silently)- Whether
parentDirshould be an absolute path or if relative paths are acceptable- What happens if directory creation fails (currently returns an error, which is good)
This would help users understand the expected behavior better.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/config.go(2 hunks)lib/sdk_private.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/sdk_private.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:
lib/config.go
🧬 Code graph analysis (1)
lib/config.go (1)
lib/sdk.go (2)
NucleiSDKOptions(37-37)NucleiEngine(59-99)
🔇 Additional comments (1)
lib/config.go (1)
6-6: LGTM! Import is necessary for the new functionality.The
osimport is required for the directory creation operations inWithTemporaryDirectory.
|
@Mzack9999 I've opted to go with the parent directory strategy as per your documentation's suggestion. When configuring The issue of cleaning up when initializing fails stands however. I'd love to hear your thoughts on this and preferably some pointers if you'd like me to address this issue. |
|
@AuditeMarlow I think the parent directory option would be preferable as it mimics exactly the current behavior, of using |
|
@Mzack9999 I think it makes sense to assume the parent directory should exist if the user chooses to configure a different temporary directory. Avoiding Additionally I think it makes sense to assume cleanup on |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/config.go (2)
568-577: Consider adding thread-safe mode check for consistency.Nine other SDK options in this file (
WithInteractshOptions,EnableStatsWithOpts,WithVerbosity,WithNetworkConfig,WithProxy,UseOutputWriter,UseStatsWriter,WithTemplateUpdateCallback,WithSandboxOptions) check for thread-safe mode and returnErrOptionsNotSupportedwhen incompatible. Since this function sets engine-level state (e.tmpDir), consider adding the same check for consistency.func WithTemporaryDirectory(parentDir string) NucleiSDKOptions { return func(e *NucleiEngine) error { + if e.mode == threadSafe { + return errkit.Wrap(ErrOptionsNotSupported, "WithTemporaryDirectory") + } tmpDir, err := os.MkdirTemp(parentDir, "nuclei-tmp-*") if err != nil { return err
568-577: Consider validating the parentDir parameter.If an empty string is passed as
parentDir,os.MkdirTempwill use the system default temporary directory, which may not be the intended behavior when a user explicitly callsWithTemporaryDirectory. Consider adding validation to ensureparentDiris non-empty.func WithTemporaryDirectory(parentDir string) NucleiSDKOptions { return func(e *NucleiEngine) error { + if parentDir == "" { + return errors.New("parentDir must not be empty") + } if e.mode == threadSafe { return errkit.Wrap(ErrOptionsNotSupported, "WithTemporaryDirectory") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/config.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/config.go (1)
lib/sdk.go (2)
NucleiSDKOptions(37-37)NucleiEngine(59-99)
2 participants
Closes #6595.
Proposed changes
The Nuclei SDK is lacking temporary directory configuration which results in Nuclei storing its template files directly under
/tmpand has no further context of cleaning up these files. By introducing temporary directory configuration to the Nuclei SDK we can keep track of where temporary files go and can clean them up when closing the Nuclei Engine.Checklist
Summary by CodeRabbit
New Features
Chores