refactor(types)!: make resume state as cache data#7042
refactor(types)!: make resume state as cache data#7042dwisiswant0 wants to merge 1 commit intodevfrom
Conversation
Resume files are runtime artifacts and SHOULD NOT be presented as persistent config. Update reset messaging to separate config vs cache and explicitly include resume state under cache cleanup. Also delete the cache dir during `-reset` to keep behavior aligned with the new semantics. Close #6792 Signed-off-by: Dwi Siswanto <git@dw1.io>
WalkthroughThe changes relocate resume configuration storage from the config directory to the cache directory, reflecting its transient nature. The reset flow is updated to delete the cache directory alongside the config directory, with warning messaging adjusted to list all three deleted areas. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/nuclei/main.go (1)
782-785: Consider adding path validation before destructive reset deletions.The
-resethandler deletes three directories without validating that the paths are safe. While the user confirmation message displays the paths being deleted—which is a strong existing safeguard—adding pre-deletion checks would provide defense-in-depth against misconfiguration or unexpected path values from environment variables (NUCLEI_CONFIG_DIR, NUCLEI_TEMPLATES_DIR).Consider validating that paths are non-empty, not system root (
/), and not duplicate before callingos.RemoveAll(). Example pattern:clean := filepath.Clean(path) if clean == "" || clean == "." || clean == string(filepath.Separator) { options.Logger.Fatal().Msgf("refusing to delete unsafe path: %q", path) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/nuclei/main.go` around lines 782 - 785, Before calling os.RemoveAll for the reset flow, validate each path returned by config.DefaultConfig.GetCacheDir(), config.DefaultConfig.GetConfigDir(), and config.DefaultConfig.GetTemplatesDir(): ensure the cleaned path (use filepath.Clean on the value) is non-empty, not "." and not the OS path separator (root), and reject duplicates among the three; if a path is unsafe, call options.Logger.Fatal().Msgf with a clear “refusing to delete unsafe path” message referencing the offending value instead of deleting it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/nuclei/main.go`:
- Around line 754-756: The reset/cleanup messaging is inconsistent: you added
the "All cache files (including resume state) at %v" line but the later success
message (the string that currently says only config + templates) and the other
occurrence still omit cache; update those success messages to mention cache as
well. Locate the print/log strings that output "All config files at %v", "All
cache files (including resume state) at %v" and "All nuclei-templates at %v" and
change the corresponding success message(s) (the one near the reset completion
and the duplicate around the 760 area) so they list config, cache, and
nuclei-templates consistently.
---
Nitpick comments:
In `@cmd/nuclei/main.go`:
- Around line 782-785: Before calling os.RemoveAll for the reset flow, validate
each path returned by config.DefaultConfig.GetCacheDir(),
config.DefaultConfig.GetConfigDir(), and config.DefaultConfig.GetTemplatesDir():
ensure the cleaned path (use filepath.Clean on the value) is non-empty, not "."
and not the OS path separator (root), and reject duplicates among the three; if
a path is unsafe, call options.Logger.Fatal().Msgf with a clear “refusing to
delete unsafe path” message referencing the offending value instead of deleting
it.
| 1. All config files at %v | ||
| 2. All cache files (including resume state) at %v | ||
| 3. All nuclei-templates at %v |
There was a problem hiding this comment.
Keep reset messaging symmetric after adding cache deletion.
Line 755 now states cache cleanup, but the success message at Line 790 still says only config + templates. Please update it to include cache for consistency.
Suggested wording update
- options.Logger.Info().Msgf("Successfully deleted all nuclei configurations files and nuclei-templates")
+ options.Logger.Info().Msgf("Successfully deleted nuclei config files, cache files, and nuclei-templates")Also applies to: 760-760
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/nuclei/main.go` around lines 754 - 756, The reset/cleanup messaging is
inconsistent: you added the "All cache files (including resume state) at %v"
line but the later success message (the string that currently says only config +
templates) and the other occurrence still omit cache; update those success
messages to mention cache as well. Locate the print/log strings that output "All
config files at %v", "All cache files (including resume state) at %v" and "All
nuclei-templates at %v" and change the corresponding success message(s) (the one
near the reset completion and the duplicate around the 760 area) so they list
config, cache, and nuclei-templates consistently.
Proposed changes
Resume files are runtime artifacts and SHOULD NOT
be presented as persistent config.
Update reset messaging to separate config vs cache
and explicitly include resume state under cache
cleanup. Also delete the cache dir during
-resetto keep behavior aligned with the new semantics.
Close #6792
Proof
N/A
Checklist
Summary by CodeRabbit