fix(loader): replace panic with error handling in template loader#6825
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
WalkthroughLoader APIs now return errors; callers were updated to check and propagate those errors. Numerous sites also replaced fmt.Sprintf(...)+WriteString with direct fmt.Fprintf to string builders; tests/benchmarks adjusted for the new loader return signatures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Replace panic with proper error return when dialers are missing,
allowing callers to handle the situation gracefully.
Changes:
- Modify LoadTemplatesWithTags to return ([]*templates.Template, error)
- Modify LoadTemplates to return ([]*templates.Template, error)
- Modify Load to return error
- Replace panic("dialers with executionId...") with fmt.Errorf
- Replace panic("could not create wait group") with fmt.Errorf
- Update all callers to handle the new error return
Callers updated:
- internal/runner/lazy.go
- internal/runner/runner.go
- internal/server/nuclei_sdk.go
- lib/multi.go
- lib/sdk.go
- cmd/integration-test/library.go
- pkg/protocols/common/automaticscan/util.go
- pkg/catalog/loader/loader_bench_test.go
Fixes projectdiscovery#6674
bimakw
force-pushed
the
fix/replace-panic-with-error-handling
branch
from
88458db to
75525ed
Compare
|
Hey, the integration test failures here are unrelated to this PR. The failing test I confirmed this by checking the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/sdk.go`:
- Around line 113-115: The new loader error returned by e.store.Load() must be
propagated and not ignored: update callers that currently ignore
e.LoadAllTemplates() (notably the places using `_ = e.LoadAllTemplates()`) to
check and return or handle its error, and add guards before dereferencing
e.store (calls to e.store.Templates() and e.store.Workflows()) so they only run
when LoadAllTemplates succeeded; also ensure ExecuteCallbackWithCtx surfaces the
underlying load error instead of masking it by returning early or wrapping the
real error. Target the functions/methods that call e.LoadAllTemplates(), any
uses of e.store.Templates()/e.store.Workflows(), and ExecuteCallbackWithCtx to
propagate or guard against load failures.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/sdk.gopkg/protocols/common/automaticscan/util.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/protocols/common/automaticscan/util.go
| if err := e.store.Load(); err != nil { | ||
| return errkit.Wrapf(err, "Could not load templates: %s", err) | ||
| } |
There was a problem hiding this comment.
Propagate the new loader error; internal callers still drop it and can still crash.
Great change at Line 113-Line 115, but the new error is still ignored at Line 123, Line 131, and Line 258 (_ = e.LoadAllTemplates()). If loading fails before e.store is ready, subsequent e.store.Templates() / e.store.Workflows() dereferences can panic, and ExecuteCallbackWithCtx may mask the real failure.
Suggested fix (propagate/guard load failures)
func (e *NucleiEngine) GetTemplates() []*templates.Template {
if !e.templatesLoaded {
- _ = e.LoadAllTemplates()
+ if err := e.LoadAllTemplates(); err != nil {
+ return nil
+ }
}
+ if e.store == nil {
+ return nil
+ }
return e.store.Templates()
}
func (e *NucleiEngine) GetWorkflows() []*templates.Template {
if !e.templatesLoaded {
- _ = e.LoadAllTemplates()
+ if err := e.LoadAllTemplates(); err != nil {
+ return nil
+ }
}
+ if e.store == nil {
+ return nil
+ }
return e.store.Workflows()
}
func (e *NucleiEngine) ExecuteCallbackWithCtx(ctx context.Context, callback ...func(event *output.ResultEvent)) error {
if !e.templatesLoaded {
- _ = e.LoadAllTemplates()
+ if err := e.LoadAllTemplates(); err != nil {
+ return err
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sdk.go` around lines 113 - 115, The new loader error returned by
e.store.Load() must be propagated and not ignored: update callers that currently
ignore e.LoadAllTemplates() (notably the places using `_ =
e.LoadAllTemplates()`) to check and return or handle its error, and add guards
before dereferencing e.store (calls to e.store.Templates() and
e.store.Workflows()) so they only run when LoadAllTemplates succeeded; also
ensure ExecuteCallbackWithCtx surfaces the underlying load error instead of
masking it by returning early or wrapping the real error. Target the
functions/methods that call e.LoadAllTemplates(), any uses of
e.store.Templates()/e.store.Workflows(), and ExecuteCallbackWithCtx to propagate
or guard against load failures.
dogancanbakir
force-pushed
the
fix/replace-panic-with-error-handling
branch
from
6cb9a2d to
99c2fad
Compare
Neo - PR Security ReviewNo security issues found Highlights
Comment |
4 participants
Summary
Replace panic with proper error return in template loader when dialers are missing, allowing callers to handle the situation gracefully.
Changes
pkg/catalog/loader/loader.goLoadTemplatesWithTagssignature:[]*templates.Template→([]*templates.Template, error)LoadTemplatessignature:[]*templates.Template→([]*templates.Template, error)Loadsignature:void→errorpanic("dialers with executionId...")withfmt.Errorf("dialers with executionId %s not found", ...)panic("could not create wait group")withfmt.Errorf("could not create wait group: %w", errWg)Callers updated
internal/runner/lazy.go- Handle error from LoadTemplatesinternal/runner/runner.go- Handle error from Loadinternal/server/nuclei_sdk.go- Handle error from Loadlib/multi.go- Handle error from Loadlib/sdk.go- Handle error from Loadcmd/integration-test/library.go- Handle error from Loadpkg/protocols/common/automaticscan/util.go- Handle error from LoadTemplatesWithTagspkg/catalog/loader/loader_bench_test.go- Update to handle two return valuesBefore/After
Before (panic):
After (graceful error):
Test Plan
go build ./...passesgo vet ./...passesgo test ./pkg/catalog/loader/...passesFixes #6674
/claim #6674
Summary by CodeRabbit
Bug Fixes
Tests
Style