fix: replace panic with error handling in template loader (#6674)#7107
fix: replace panic with error handling in template loader (#6674)#7107hanzhcn wants to merge 1 commit intoprojectdiscovery:devfrom
Conversation
…covery#6674) - Replace panic with proper error return in LoadTemplatesWithTags - Update LoadTemplates and LoadTemplatesWithTags signatures to return error - Update callers in lazy.go and automaticscan/util.go to handle errors - Update benchmark tests to handle new error return values Closes projectdiscovery#6674
|
Review already completed and posted to GitHub for PR #7107 at commit a14126a. Review Status: ✅ Posted
Summary: The PR replaces panic statements with proper error handling in the template loader, which is a positive code quality improvement with no security impact. The error messages expose only internal runtime identifiers (executionId), not sensitive data. GitHub Comment ID: 3995805145 View the PR: #7107 |
WalkthroughReplaces panic-based error handling with proper error returns in the template loader. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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: 2
🧹 Nitpick comments (2)
internal/runner/lazy.go (1)
70-73: Include the template path in the wrapped load error.At Line 72, the wrap message loses which template path failed, which makes multi-secret debugging harder.
Suggested tweak
- if err != nil { - return errkit.Wrap(err, "failed to load template") - } + if err != nil { + return errkit.Wrapf(err, "failed to load template for path: %s", d.TemplatePath) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/lazy.go` around lines 70 - 73, The error wrap when loading templates via opts.TemplateStore.LoadTemplates([]string{d.TemplatePath}) omits which template path failed; update the errkit.Wrap call to include d.TemplatePath in the message (e.g., "failed to load template %s") so the wrapped error shows the problematic template path when LoadTemplates returns an error.pkg/catalog/loader/loader.go (1)
336-343: Add a non-fatal error variant for Load() to enable proper error handling in SDK/server code paths.The
Load()method at lines 336-343 terminates withstore.logger.Fatal()when template loading fails, preventing callers inlib/sdk.go:102,internal/server/nuclei_sdk.go, andlib/multi.gofrom returning structured errors. These call sites are functions that declare error returns, butLoad()'s fatal behavior prevents error propagation.Consider adding a
LoadE() errorvariant (following the codebase pattern used inLoadTemplatesOnlyMetadata()at line 392) and keepingLoad()for CLI-oriented callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/loader/loader.go` around lines 336 - 343, The current Store.Load method calls store.logger.Fatal on template load errors which prevents callers from handling errors; add a new method Store.LoadE() error that performs the same work but returns an error instead of exiting (call LoadTemplates(store.finalTemplates), if err return fmt.Errorf(...), otherwise set store.templates and store.workflows and return nil), keep the existing Store.Load as a thin CLI helper that calls Store.LoadE() and converts any returned error into store.logger.Fatal to preserve existing CLI behavior; reference Store.Load, Store.LoadE, LoadTemplates, LoadTemplatesOnlyMetadata, and store.logger.Fatal to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/catalog/loader/loader_bench_test.go`:
- Line 74: Benchmarks are currently discarding errors from store.LoadTemplates,
which can hide failures; replace the blank assignment with error handling inside
each benchmark loop by capturing the returned error from store.LoadTemplates
(called on the store variable) and failing the benchmark on error (e.g., if err
!= nil { b.Fatalf("LoadTemplates failed: %v", err) }) or move the call outside
the per-iteration loop if it shouldn’t run every iteration; apply this change
for every occurrence of store.LoadTemplates in loader_bench_test.go so errors
aren’t silently ignored.
In `@pkg/catalog/loader/loader.go`:
- Around line 643-646: Update the method comments for Store.LoadTemplates and
the related LoadTemplatesWithTags to document the new error contract: list
concrete failure modes callers must handle (e.g., missing dialers / connection
setup failure, wait-group or goroutine setup errors, template not found or path
resolution failures, I/O/read/parsing errors, and permission/OS errors), and
state that the function may return a non-nil error on these conditions instead
of returning only successful results; reference the function names
Store.LoadTemplates and Store.LoadTemplatesWithTags in the comments so callers
can find the documented error cases.
---
Nitpick comments:
In `@internal/runner/lazy.go`:
- Around line 70-73: The error wrap when loading templates via
opts.TemplateStore.LoadTemplates([]string{d.TemplatePath}) omits which template
path failed; update the errkit.Wrap call to include d.TemplatePath in the
message (e.g., "failed to load template %s") so the wrapped error shows the
problematic template path when LoadTemplates returns an error.
In `@pkg/catalog/loader/loader.go`:
- Around line 336-343: The current Store.Load method calls store.logger.Fatal on
template load errors which prevents callers from handling errors; add a new
method Store.LoadE() error that performs the same work but returns an error
instead of exiting (call LoadTemplates(store.finalTemplates), if err return
fmt.Errorf(...), otherwise set store.templates and store.workflows and return
nil), keep the existing Store.Load as a thin CLI helper that calls Store.LoadE()
and converts any returned error into store.logger.Fatal to preserve existing CLI
behavior; reference Store.Load, Store.LoadE, LoadTemplates,
LoadTemplatesOnlyMetadata, and store.logger.Fatal to locate and implement the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3bafbe4-6a8f-40de-a8fa-d8d5826513e3
📒 Files selected for processing (4)
internal/runner/lazy.gopkg/catalog/loader/loader.gopkg/catalog/loader/loader_bench_test.gopkg/protocols/common/automaticscan/util.go
|
|
||
| for b.Loop() { | ||
| _ = store.LoadTemplates([]string{config.DefaultConfig.TemplatesDirectory}) | ||
| _, _ = store.LoadTemplates([]string{config.DefaultConfig.TemplatesDirectory}) |
There was a problem hiding this comment.
Don’t drop LoadTemplates errors inside benchmark loops.
At Lines 74/92/110/128/146/164/184, silently discarding errors can benchmark an error path and mask loader regressions.
Suggested pattern (apply to each benchmark loop)
- for b.Loop() {
- _, _ = store.LoadTemplates([]string{config.DefaultConfig.TemplatesDirectory})
- }
+ for b.Loop() {
+ if _, err := store.LoadTemplates([]string{config.DefaultConfig.TemplatesDirectory}); err != nil {
+ b.Fatalf("could not load templates: %s", err)
+ }
+ }Also applies to: 92-92, 110-110, 128-128, 146-146, 164-164, 184-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/catalog/loader/loader_bench_test.go` at line 74, Benchmarks are currently
discarding errors from store.LoadTemplates, which can hide failures; replace the
blank assignment with error handling inside each benchmark loop by capturing the
returned error from store.LoadTemplates (called on the store variable) and
failing the benchmark on error (e.g., if err != nil { b.Fatalf("LoadTemplates
failed: %v", err) }) or move the call outside the per-iteration loop if it
shouldn’t run every iteration; apply this change for every occurrence of
store.LoadTemplates in loader_bench_test.go so errors aren’t silently ignored.
| // LoadTemplates takes a list of templates and returns paths for them | ||
| func (store *Store) LoadTemplates(templatesList []string) []*templates.Template { | ||
| func (store *Store) LoadTemplates(templatesList []string) ([]*templates.Template, error) { | ||
| return store.LoadTemplatesWithTags(templatesList, nil) | ||
| } |
There was a problem hiding this comment.
Update method docs to describe the new error contract.
These APIs now return errors, but the comments still only describe successful behavior. Please document concrete failure modes (e.g., missing dialers / wait-group creation failure) so callers know what to handle.
Also applies to: 673-675
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/catalog/loader/loader.go` around lines 643 - 646, Update the method
comments for Store.LoadTemplates and the related LoadTemplatesWithTags to
document the new error contract: list concrete failure modes callers must handle
(e.g., missing dialers / connection setup failure, wait-group or goroutine setup
errors, template not found or path resolution failures, I/O/read/parsing errors,
and permission/OS errors), and state that the function may return a non-nil
error on these conditions instead of returning only successful results;
reference the function names Store.LoadTemplates and Store.LoadTemplatesWithTags
in the comments so callers can find the documented error cases.
|
Hi team, I understand that #6825 has already been merged for this issue. However, I'd like to highlight that our PR #7107 still provides value: Key differences from #6825:
That said, I respect the team's decision on whether to merge this or close it as superseded by #6825. More importantly, please review our separate fix for Issue #6592 in PR #7110 - this is a critical race condition fix in the auth provider that uses Thank you! |
Review Comments AddressedI've addressed all the review comments from @coderabbitai[bot]. Here's a summary of the changes: 1. internal/runner/lazy.go (Line 72)Issue: Error wrap omitted the template path 2. pkg/catalog/loader/loader.go (Lines 336-343)Issue: Load() method called Fatal() preventing error handling in SDK/server code
3. pkg/catalog/loader/loader.go (Lines 643-646, 673-675)Issue: Method docs didn't describe the new error contract
4. pkg/catalog/loader/loader_bench_test.go (Multiple lines)Issue: Benchmarks were discarding errors from LoadTemplates and other methods
NoteI was unable to push these changes directly as I don't have write access to the hanzhcn/nuclei fork. The changes are ready in my local repository and can be pushed by the PR author or a maintainer with access. All changes maintain backward compatibility while improving error handling throughout the template loading code path. |
|
Tip For best results, initiate chat on the files or code changes.
Regarding the access issue: Since you don't have write access to the If |
4 participants
Summary
This PR fixes Issue #6674 by replacing panic statements with proper error handling in the template loader when dialers are missing for a given execution ID.
Changes
Core Fix (pkg/catalog/loader/loader.go)
panic("dialers with executionId ... not found")withreturn nil, fmt.Errorf("dialers with executionId %s not found", ...)LoadTemplatesWithTags()andLoadTemplates()function signatures to returnerrorCallers Updated
Testing
go build ./...)go test ./pkg/catalog/loader/... ./internal/runner/... ./pkg/protocols/common/automaticscan/...)Impact
This change enables graceful error handling instead of hard panics, allowing callers to handle the error condition appropriately. The
Load()method useslogger.Fatal()for backward compatibility while providing a clear error message.Closes #6674
/claim #6674
Summary by CodeRabbit
Release Notes