fix(loader): replace panic with error handling in template loader (#6674)#7085
Open
Madhavan-Deepak wants to merge 5 commits intoprojectdiscovery:devfrom
Open
fix(loader): replace panic with error handling in template loader (#6674)#7085Madhavan-Deepak wants to merge 5 commits intoprojectdiscovery:devfrom
Madhavan-Deepak wants to merge 5 commits intoprojectdiscovery:devfrom
Conversation
Neo - PR Security ReviewNo security issues found Highlights
Hardening Notes
Comment |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdd explicit error returns and propagate template-loading failures across the codebase; callers now check and return wrapped errors when Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/catalog/loader/loader_test.go (1)
125-127: Consider using require.NoError for consistency.The existing tests in this file use
require.Nil(t, err, ...)for error assertions. While the current approach works, consider using testify's assertion methods for consistency.♻️ Use testify assertions for consistency
store, err := New(&Config{ Templates: []string{"test"}, Catalog: catalog, ExecutorOptions: options, Logger: gologger.DefaultLogger, }) - if err != nil { - t.Fatalf("unexpected loader creation error: %v", err) - } + require.NoError(t, err, "unexpected loader creation error") - _, err = store.LoadTemplatesWithTags([]string{"test"}, []string{"test"}) - if err == nil { - t.Fatal("expected error when dialers are missing, got nil") - } - if !strings.Contains(err.Error(), "dialers with executionId") { - t.Errorf("unexpected error message: %v", err) - } + _, err = store.LoadTemplatesWithTags([]string{"test"}, []string{"test"}) + require.Error(t, err, "expected error when dialers are missing") + require.Contains(t, err.Error(), "dialers with executionId", "unexpected error message")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/loader/loader_test.go` around lines 125 - 127, Replace the plain testing fatal check with testify's assertion to match the file's style: change the t.Fatalf("unexpected loader creation error: %v", err) check to require.NoError(t, err) (or require.Nil(t, err) if the file uses Nil elsewhere) in the test that creates the loader so the error variable `err` is asserted via `require`; also ensure the `require` package is imported in the test file if not already.lib/sdk.go (1)
122-124: Silent error discard inGetTemplatesmay hide loading failures.When
LoadAllTemplates()fails, the error is silently discarded with_ =. This could lead to unexpected behavior wherestore.Templates()is called on an unloaded/partially loaded store. Consider logging the error or returning it.♻️ Consider logging the error
func (e *NucleiEngine) GetTemplates() []*templates.Template { if !e.templatesLoaded { - _ = e.LoadAllTemplates() + if err := e.LoadAllTemplates(); err != nil { + gologger.DefaultLogger.Warning().Msgf("failed to load templates: %s", err) + } } return e.store.Templates() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk.go` around lines 122 - 124, GetTemplates currently discards errors from e.LoadAllTemplates() and proceeds to call store.Templates(), which can hide failures; change the code in GetTemplates so that the result of e.LoadAllTemplates() is handled instead of ignored: call err := e.LoadAllTemplates() and either return the error up the call chain or log it (using the existing logger/context) before continuing, and only call store.Templates() when e.templatesLoaded is true or after a successful LoadAllTemplates(); reference symbols: GetTemplates, e.templatesLoaded, e.LoadAllTemplates(), and store.Templates() to locate the change.lib/multi.go (1)
153-155: Minor: Redundant error formatting in errkit.Wrapf.The format string
"Could not load templates: %s"includes%swhich will print the error, buterrkit.Wrapfalready appends the wrapped error. This may result in duplicate error text in the message. Consider simplifying to avoid redundancy.♻️ Simplify error wrapping
if err := store.Load(); err != nil { - return errkit.Wrapf(err, "Could not load templates: %s", err) + return errkit.Wrap(err, "could not load templates") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/multi.go` around lines 153 - 155, The error wrap in the store.Load() failure uses errkit.Wrapf(err, "Could not load templates: %s", err) which duplicates the error text; update the call to remove the redundant "%s" and just provide context (e.g., errkit.Wrapf(err, "Could not load templates") or use errkit.Wrap(err, "Could not load templates")) so that the wrapped error is appended only once; locate this in the function where store.Load() is called and replace the Wrapf invocation accordingly.pkg/catalog/loader/loader.go (1)
723-726: Use a sentinel error for missing dialers to avoid brittle string matching.At Line 725, returning only a formatted string forces downstream checks/tests to rely on message contents. Prefer a typed/sentinel error wrapped with context so callers can use
errors.Is.Suggested refactor
var ( TrustedTemplateDomains = []string{"cloud.projectdiscovery.io"} + ErrDialersNotFound = errors.New("dialers not found for execution id") ) @@ dialers := protocolstate.GetDialersWithId(typesOpts.ExecutionId) if dialers == nil { - return nil, fmt.Errorf("dialers with executionId %s not found", typesOpts.ExecutionId) + return nil, fmt.Errorf("%w: %s", ErrDialersNotFound, typesOpts.ExecutionId) }🤖 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 723 - 726, Replace the plain formatted error returned when dialers are nil with a wrapped sentinel error so callers can reliably use errors.Is; define a package-level error like ErrDialersNotFound and return fmt.Errorf("%w: dialers with executionId %s not found", ErrDialersNotFound, typesOpts.ExecutionId) in the block where protocolstate.GetDialersWithId(typesOpts.ExecutionId) returns nil, and update any callers/tests to use errors.Is(err, ErrDialersNotFound) to detect this condition.
🤖 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`:
- Around line 74-75: The benchmark currently ignores the error returned by
store.LoadTemplates, causing failed loads to be measured; update each call (in
loader_bench_test.go occurrences around the LoadTemplates call) to capture the
returned error and fail the benchmark immediately (e.g., call b.Fatalf or
b.FailNow with the error) when err != nil so benchmark iterations stop on a
template-loading failure instead of measuring an error path; apply this change
to all occurrences at lines around the shown call.
---
Nitpick comments:
In `@lib/multi.go`:
- Around line 153-155: The error wrap in the store.Load() failure uses
errkit.Wrapf(err, "Could not load templates: %s", err) which duplicates the
error text; update the call to remove the redundant "%s" and just provide
context (e.g., errkit.Wrapf(err, "Could not load templates") or use
errkit.Wrap(err, "Could not load templates")) so that the wrapped error is
appended only once; locate this in the function where store.Load() is called and
replace the Wrapf invocation accordingly.
In `@lib/sdk.go`:
- Around line 122-124: GetTemplates currently discards errors from
e.LoadAllTemplates() and proceeds to call store.Templates(), which can hide
failures; change the code in GetTemplates so that the result of
e.LoadAllTemplates() is handled instead of ignored: call err :=
e.LoadAllTemplates() and either return the error up the call chain or log it
(using the existing logger/context) before continuing, and only call
store.Templates() when e.templatesLoaded is true or after a successful
LoadAllTemplates(); reference symbols: GetTemplates, e.templatesLoaded,
e.LoadAllTemplates(), and store.Templates() to locate the change.
In `@pkg/catalog/loader/loader_test.go`:
- Around line 125-127: Replace the plain testing fatal check with testify's
assertion to match the file's style: change the t.Fatalf("unexpected loader
creation error: %v", err) check to require.NoError(t, err) (or require.Nil(t,
err) if the file uses Nil elsewhere) in the test that creates the loader so the
error variable `err` is asserted via `require`; also ensure the `require`
package is imported in the test file if not already.
In `@pkg/catalog/loader/loader.go`:
- Around line 723-726: Replace the plain formatted error returned when dialers
are nil with a wrapped sentinel error so callers can reliably use errors.Is;
define a package-level error like ErrDialersNotFound and return fmt.Errorf("%w:
dialers with executionId %s not found", ErrDialersNotFound,
typesOpts.ExecutionId) in the block where
protocolstate.GetDialersWithId(typesOpts.ExecutionId) returns nil, and update
any callers/tests to use errors.Is(err, ErrDialersNotFound) to detect this
condition.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/integration-test/library.gointernal/runner/lazy.gointernal/runner/runner.gointernal/server/nuclei_sdk.golib/multi.golib/sdk.gopkg/catalog/loader/loader.gopkg/catalog/loader/loader_bench_test.gopkg/catalog/loader/loader_test.gopkg/protocols/common/automaticscan/util.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/sdk.go (2)
139-148: Same pattern as GetTemplates — error is logged but not surfaced to caller.Consider aligning the error handling approach in both
GetTemplatesandGetWorkflowsif you decide to change one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk.go` around lines 139 - 148, The GetWorkflows flow currently logs LoadAllTemplates errors but swallows them by returning nil; change it to surface the error to the caller like GetTemplates does — when e.LoadAllTemplates() returns an error, log via e.Logger (if present) and return that error (or propagate it) instead of returning nil; update the e.LoadAllTemplates() error handling in the function containing this snippet and ensure it matches the error-return behavior used by GetTemplates so callers can react to the failure.
123-132: Error handling returns nil without distinguishing failure from empty results.When
LoadAllTemplates()fails, this method logs the error (if Logger is set) and returnsnil. Callers cannot distinguish between "no templates found" and "loading failed." Consider returning an error or documenting thatnilmay indicate a loading failure, not just an empty template set.That said, this maintains backward compatibility since the method signature wasn't changed to return an error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk.go` around lines 123 - 132, When LoadAllTemplates() fails the code currently logs and returns nil, which hides whether nil means "no templates" or "load failed"; instead record the failure on the receiver and keep the return value stable: set a new field e.loadError (or similar) to the returned err inside the if-block (and still log via e.Logger.Error()), and add an accessor method GetLoadError() or LastLoadError() so callers can distinguish a load failure from an empty result without changing the method signature; update any callsites/tests to check GetLoadError() when they need to know if loading failed.pkg/catalog/loader/loader_test.go (1)
109-135: Good regression test for the dialers-not-found error case.This test correctly validates that
LoadTemplatesWithTagsreturnsErrDialersNotFoundwhen dialers for the given execution ID don't exist. The use ofrequire.ErrorIsproperly checks the error chain.Minor suggestion: The nil check at lines 128-130 is redundant since
require.ErrorIsat line 131 will also fail iferrisnil.♻️ Optional: Remove redundant nil check
_, err = store.LoadTemplatesWithTags([]string{"test"}, []string{"test"}) - if err == nil { - t.Fatal("expected error when dialers are missing, got nil") - } require.ErrorIs(t, err, ErrDialersNotFound) if !strings.Contains(err.Error(), "dialers with executionId") { t.Errorf("unexpected error message: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/loader/loader_test.go` around lines 109 - 135, The test TestLoadTemplates_NoDialers contains a redundant nil-check (the t.Fatal block that checks if err == nil) before the require.ErrorIs assertion; remove the explicit if err == nil { t.Fatal(...) } block and leave the require.ErrorIs(t, err, ErrDialersNotFound) and the subsequent error message assertion so the test remains concise and still fails correctly when err is nil or not the expected error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/sdk.go`:
- Around line 139-148: The GetWorkflows flow currently logs LoadAllTemplates
errors but swallows them by returning nil; change it to surface the error to the
caller like GetTemplates does — when e.LoadAllTemplates() returns an error, log
via e.Logger (if present) and return that error (or propagate it) instead of
returning nil; update the e.LoadAllTemplates() error handling in the function
containing this snippet and ensure it matches the error-return behavior used by
GetTemplates so callers can react to the failure.
- Around line 123-132: When LoadAllTemplates() fails the code currently logs and
returns nil, which hides whether nil means "no templates" or "load failed";
instead record the failure on the receiver and keep the return value stable: set
a new field e.loadError (or similar) to the returned err inside the if-block
(and still log via e.Logger.Error()), and add an accessor method GetLoadError()
or LastLoadError() so callers can distinguish a load failure from an empty
result without changing the method signature; update any callsites/tests to
check GetLoadError() when they need to know if loading failed.
In `@pkg/catalog/loader/loader_test.go`:
- Around line 109-135: The test TestLoadTemplates_NoDialers contains a redundant
nil-check (the t.Fatal block that checks if err == nil) before the
require.ErrorIs assertion; remove the explicit if err == nil { t.Fatal(...) }
block and leave the require.ErrorIs(t, err, ErrDialersNotFound) and the
subsequent error message assertion so the test remains concise and still fails
correctly when err is nil or not the expected error.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/multi.golib/sdk.gopkg/catalog/loader/loader.gopkg/catalog/loader/loader_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
This PR addresses issue #6674 by replacing explicit panics with returned errors in the template loader.
pkg/catalog/loader/loader.goto return a formatted error instead of crashing when dialers are nil.syncutil.Newto safely propagate errors back to the caller.runner.goandnuclei_sdk.gohandle these new error returns gracefully.Proof
TestLoadTemplates_NoDialersinpkg/catalog/loader/loader_test.goto verify the fix.Checklist
/claim #6674
Summary by CodeRabbit
Bug Fixes
Refactor
Tests