fix: remove nil interface set in createEphemeralObjects to avoid panic#6944
Open
maxwolf8852 wants to merge 5 commits intoprojectdiscovery:devfrom
Open
fix: remove nil interface set in createEphemeralObjects to avoid panic#6944maxwolf8852 wants to merge 5 commits intoprojectdiscovery:devfrom
maxwolf8852 wants to merge 5 commits intoprojectdiscovery:devfrom
Conversation
Contributor
|
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:
WalkthroughRelocated the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/multi.go (1)
37-37: DeadRateLimiterinitializer in struct literal.
RateLimiter: base.rateLimiter(line 37) is unconditionally overwritten at line 54 byutils.GetRateLimiter(...)with no intervening read, so the struct-literal assignment is never observed.♻️ Proposed cleanup
u.executerOpts = &protocols.ExecutorOptions{ Output: base.customWriter, Options: opts, Progress: base.customProgress, Catalog: base.catalog, IssuesClient: base.rc, - RateLimiter: base.rateLimiter, Interactsh: base.interactshClient, Colorizer: aurora.NewAurora(true), ResumeCfg: types.NewResumeCfg(), Parser: base.parser, Browser: base.browserInstance, }Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/multi.go` at line 37, The struct literal sets RateLimiter: base.rateLimiter but that value is immediately overwritten by the subsequent utils.GetRateLimiter(...) call, making the initial assignment dead; remove the redundant RateLimiter: base.rateLimiter entry from the struct literal (or, alternatively, only set RateLimiter to base.rateLimiter if utils.GetRateLimiter(...) returns nil) so that the final RateLimiter used comes only from utils.GetRateLimiter(...) or is fallback-assigned consistently; look for the struct construction where base.rateLimiter is used and the later call to utils.GetRateLimiter to apply this change.
🤖 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/multi.go`:
- Line 37: The struct literal sets RateLimiter: base.rateLimiter but that value
is immediately overwritten by the subsequent utils.GetRateLimiter(...) call,
making the initial assignment dead; remove the redundant RateLimiter:
base.rateLimiter entry from the struct literal (or, alternatively, only set
RateLimiter to base.rateLimiter if utils.GetRateLimiter(...) returns nil) so
that the final RateLimiter used comes only from utils.GetRateLimiter(...) or is
fallback-assigned consistently; look for the struct construction where
base.rateLimiter is used and the later call to utils.GetRateLimiter to apply
this change.
Neo - PR Security ReviewNo security issues found Highlights
Comment |
dwisiswant0
force-pushed
the
fix/noHostErrors-panic
branch
from
12bc26b to
708ff29
Compare
dwisiswant0
approved these changes
Feb 24, 2026
dwisiswant0
force-pushed
the
fix/noHostErrors-panic
branch
from
708ff29 to
b987c4b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/sdk_test.go (2)
65-67: Redundantt.Cleanupfor temp file removal.
os.CreateTemp(t.TempDir(), ...)places the file inside the directory created byt.TempDir(), which the testing package already deletes automatically at the end of the test. The extraos.Removecleanup is a no-op.♻️ Suggested simplification
- t.Cleanup(func() { - _ = os.Remove(tempTemplate.Name()) - }) -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk_test.go` around lines 65 - 67, Redundant t.Cleanup: remove the t.Cleanup wrapper that calls os.Remove(tempTemplate.Name()) because the file was created with os.CreateTemp(t.TempDir(), ...) and the test framework will delete t.TempDir() automatically; simply delete the t.Cleanup(func() { _ = os.Remove(tempTemplate.Name()) }) block (references: t.Cleanup, os.Remove, os.CreateTemp, t.TempDir, tempTemplate.Name()).
98-108: Userequire.NoErrordirectly; drop the commented-out block.The
if err != nil { require.NoError(...) }pattern appears three times and is inconsistent with the existing test style. A barerequire.NoErroris equivalent, clearer, and already the project convention. The commented-out callback (lines 102–104) is a debug artifact.♻️ Suggested simplification
- if err := ne.GlobalLoadAllTemplates(); err != nil { - require.NoError(t, err, "could not load templates") - } - - // ne.GlobalResultCallback(func(event *output.ResultEvent) { - // fmt.Println(event.Host, event.Info.SeverityHolder.Severity) - // }) - - if err := ne.ExecuteNucleiWithOptsCtx(context.TODO(), []string{"http://scanme.sh"}); err != nil { - require.NoError(t, err, "nuclei execution should not return an error") - } + require.NoError(t, ne.GlobalLoadAllTemplates(), "could not load templates") + require.NoError(t, ne.ExecuteNucleiWithOptsCtx(context.TODO(), []string{"http://scanme.sh"}), "nuclei execution should not return an error")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk_test.go` around lines 98 - 108, Replace the explicit if err != nil { require.NoError(t, err, "...") } checks with direct calls to require.NoError(t, err, "...") for GlobalLoadAllTemplates and ExecuteNucleiWithOptsCtx to match project test style (use the existing function names GlobalLoadAllTemplates and ExecuteNucleiWithOptsCtx to locate the assertions), and remove the commented-out GlobalResultCallback debug block entirely.
🤖 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_test.go`:
- Around line 93-96: The test creates a nuclei engine with
nuclei.NewThreadSafeNucleiEngineCtx and stores it in variable ne but never
closes it, leaking goroutines/resources; add a defer ne.Close() immediately
after successful creation (mirroring other tests like
TestContextCancelNucleiEngine and TestHeadlessOptionInitialization) so the
engine is always shut down when the test finishes.
---
Nitpick comments:
In `@lib/sdk_test.go`:
- Around line 65-67: Redundant t.Cleanup: remove the t.Cleanup wrapper that
calls os.Remove(tempTemplate.Name()) because the file was created with
os.CreateTemp(t.TempDir(), ...) and the test framework will delete t.TempDir()
automatically; simply delete the t.Cleanup(func() { _ =
os.Remove(tempTemplate.Name()) }) block (references: t.Cleanup, os.Remove,
os.CreateTemp, t.TempDir, tempTemplate.Name()).
- Around line 98-108: Replace the explicit if err != nil { require.NoError(t,
err, "...") } checks with direct calls to require.NoError(t, err, "...") for
GlobalLoadAllTemplates and ExecuteNucleiWithOptsCtx to match project test style
(use the existing function names GlobalLoadAllTemplates and
ExecuteNucleiWithOptsCtx to locate the assertions), and remove the commented-out
GlobalResultCallback debug block entirely.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/multi.golib/sdk_test.go
✅ Files skipped from review due to trivial changes (1)
- lib/multi.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/sdk_test.go (1)
93-96:⚠️ Potential issue | 🟡 MinorMissing
defer ne.Close()— resource leak.Both existing tests call
defer ne.Close()immediately after a successful engine creation. This test omits it, leaving goroutines and network resources open until the process exits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk_test.go` around lines 93 - 96, The test creates a nuclei engine with nuclei.NewThreadSafeNucleiEngineCtx and assigns it to ne but never closes it; add a deferred close immediately after successful creation — e.g., after checking err is nil, call defer ne.Close() — so the engine's goroutines and network resources are cleaned up; update the test around the nuclei.NewThreadSafeNucleiEngineCtx call to include defer ne.Close().
🧹 Nitpick comments (3)
lib/sdk_test.go (3)
65-67: Redundantt.Cleanup—t.TempDir()already handles removal.
os.CreateTemp(t.TempDir(), ...)stores the file inside a directory created byt.TempDir(), which automatically registers a cleanup that removes the entire directory tree when the test ends. The manualos.Removeis dead code.♻️ Proposed simplification
- t.Cleanup(func() { - _ = os.Remove(tempTemplate.Name()) - }) -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk_test.go` around lines 65 - 67, Remove the redundant manual cleanup that calls t.Cleanup with os.Remove(tempTemplate.Name()) because the file was created via os.CreateTemp(t.TempDir(), ...) and t.TempDir() already registers cleanup for the temp directory; locate the t.Cleanup call referencing os.Remove and delete that block so cleanup is solely handled by t.TempDir().
102-104: Remove commented-out debug code.Dead code should not be committed; if the callback is useful for future debugging, track it in an issue instead.
♻️ Proposed fix
- // ne.GlobalResultCallback(func(event *output.ResultEvent) { - // fmt.Println(event.Host, event.Info.SeverityHolder.Severity) - // }) -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk_test.go` around lines 102 - 104, Remove the commented-out debug callback in lib/sdk_test.go: delete the two lines containing the ne.GlobalResultCallback(...) block (the commented call and its fmt.Println) so no dead debug code remains; if you want to preserve this snippet for future use, open an issue referencing GlobalResultCallback and output.ResultEvent instead of leaving commented code in the test.
93-96: Non-idiomaticif err != nil { require.NoError(...) }— three occurrences.
require.NoErroralready short-circuits on nil (t.FailNowis only called whenerr != nil), so theif err != nilguard is always redundant. Every otherrequire.NoErrorcall in this file (lines 22, 52, 63, 82, 83) is written without the guard. Mixing patterns reduces readability.♻️ Proposed fix — apply at all three sites
ne, err := nuclei.NewThreadSafeNucleiEngineCtx(context.TODO(), nuclei.WithOptions(options)) - if err != nil { - require.NoError(t, err, "could not create nuclei engine") - } + require.NoError(t, err, "could not create nuclei engine") + defer ne.Close()- if err := ne.GlobalLoadAllTemplates(); err != nil { - require.NoError(t, err, "could not load templates") - } + require.NoError(t, ne.GlobalLoadAllTemplates(), "could not load templates")- if err := ne.ExecuteNucleiWithOptsCtx(context.TODO(), []string{"scanme.sh"}); err != nil { - require.NoError(t, err, "nuclei execution should not return an error") - } + require.NoError(t, ne.ExecuteNucleiWithOptsCtx(context.TODO(), []string{"scanme.sh"}), "nuclei execution should not return an error")Also applies to: 98-100, 106-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk_test.go` around lines 93 - 96, Remove the redundant if err != nil guards wrapping require.NoError calls: when creating the nuclei engine via nuclei.NewThreadSafeNucleiEngineCtx (and the two other identical sites), call require.NoError(t, err, "could not create nuclei engine") directly without the surrounding if block so the test helper short-circuits correctly and matches the other require.NoError usages in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/sdk_test.go`:
- Around line 93-96: The test creates a nuclei engine with
nuclei.NewThreadSafeNucleiEngineCtx and assigns it to ne but never closes it;
add a deferred close immediately after successful creation — e.g., after
checking err is nil, call defer ne.Close() — so the engine's goroutines and
network resources are cleaned up; update the test around the
nuclei.NewThreadSafeNucleiEngineCtx call to include defer ne.Close().
---
Nitpick comments:
In `@lib/sdk_test.go`:
- Around line 65-67: Remove the redundant manual cleanup that calls t.Cleanup
with os.Remove(tempTemplate.Name()) because the file was created via
os.CreateTemp(t.TempDir(), ...) and t.TempDir() already registers cleanup for
the temp directory; locate the t.Cleanup call referencing os.Remove and delete
that block so cleanup is solely handled by t.TempDir().
- Around line 102-104: Remove the commented-out debug callback in
lib/sdk_test.go: delete the two lines containing the
ne.GlobalResultCallback(...) block (the commented call and its fmt.Println) so
no dead debug code remains; if you want to preserve this snippet for future use,
open an issue referencing GlobalResultCallback and output.ResultEvent instead of
leaving commented code in the test.
- Around line 93-96: Remove the redundant if err != nil guards wrapping
require.NoError calls: when creating the nuclei engine via
nuclei.NewThreadSafeNucleiEngineCtx (and the two other identical sites), call
require.NoError(t, err, "could not create nuclei engine") directly without the
surrounding if block so the test helper short-circuits correctly and matches the
other require.NoError usages in the file.
Signed-off-by: Dwi Siswanto <git@dw1.io>
dwisiswant0
force-pushed
the
fix/noHostErrors-panic
branch
from
b987c4b to
002b58d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/sdk_test.go (3)
65-67: Redundantt.Cleanup—t.TempDir()already owns cleanup.
t.TempDir()registers its ownt.Cleanupthat removes the entire directory (including the file inside) when the test ends. The explicitos.Removeis never needed and can be dropped entirely.🧹 Proposed fix
- t.Cleanup(func() { - _ = os.Remove(tempTemplate.Name()) - }) -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk_test.go` around lines 65 - 67, Remove the redundant cleanup: delete the t.Cleanup wrapper that calls os.Remove(tempTemplate.Name()) since t.TempDir() already registers cleanup for the directory and its contents; simply rely on t.TempDir() and keep the tempTemplate variable usage but remove the explicit os.Remove call and its surrounding t.Cleanup block (referencing tempTemplate and t.TempDir()).
104-106: Remove commented-outGlobalResultCallbackblock.Debug/example code left as a comment should be cleaned up before merging. If the intent is to show usage, a code comment (not commented-out code) or a separate example file would be more appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk_test.go` around lines 104 - 106, Remove the commented-out debug/example block that calls ne.GlobalResultCallback and prints event.Host and event.Info.SeverityHolder.Severity; either delete those commented lines entirely or move their intent into a proper example/test file or explanatory comment. Locate the commented block referencing GlobalResultCallback and output.ResultEvent in the test (the lines starting with // ne.GlobalResultCallback(...)) and remove it so the test file contains no leftover commented-out code.
93-96: Unwrap the redundantif err != nilguard aroundrequire.NoError.
require.NoError(t, err, ...)already short-circuits the test whenerr != nil; the enclosingifadds no value and diverges from the idiomatic style used everywhere else in this file.♻️ Proposed fix (all three sites)
- ne, err := nuclei.NewThreadSafeNucleiEngineCtx(context.TODO(), nuclei.WithOptions(options)) - if err != nil { - require.NoError(t, err, "could not create nuclei engine") - } + ne, err := nuclei.NewThreadSafeNucleiEngineCtx(context.TODO(), nuclei.WithOptions(options)) + require.NoError(t, err, "could not create nuclei engine") defer ne.Close() - if err := ne.GlobalLoadAllTemplates(); err != nil { - require.NoError(t, err, "could not load templates") - } + require.NoError(t, ne.GlobalLoadAllTemplates(), "could not load templates") - if err := ne.ExecuteNucleiWithOptsCtx(context.TODO(), []string{"scanme.sh"}); err != nil { - require.NoError(t, err, "nuclei execution should not return an error") - } + require.NoError(t, ne.ExecuteNucleiWithOptsCtx(context.TODO(), []string{"scanme.sh"}), "nuclei execution should not return an error")Also applies to: 100-102, 108-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk_test.go` around lines 93 - 96, The current pattern wraps require.NoError(t, err, ...) inside an unnecessary if err != nil { ... } guard around calls to nuclei.NewThreadSafeNucleiEngineCtx; remove the redundant if checks and directly call require.NoError(t, err, "could not create nuclei engine") (and analogous messages) after the NewThreadSafeNucleiEngineCtx calls so the test short-circuits idiomatically; update all three occurrences that follow this pattern (look for NewThreadSafeNucleiEngineCtx + err + require.NoError usage).
🤖 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_test.go`:
- Around line 65-67: Remove the redundant cleanup: delete the t.Cleanup wrapper
that calls os.Remove(tempTemplate.Name()) since t.TempDir() already registers
cleanup for the directory and its contents; simply rely on t.TempDir() and keep
the tempTemplate variable usage but remove the explicit os.Remove call and its
surrounding t.Cleanup block (referencing tempTemplate and t.TempDir()).
- Around line 104-106: Remove the commented-out debug/example block that calls
ne.GlobalResultCallback and prints event.Host and
event.Info.SeverityHolder.Severity; either delete those commented lines entirely
or move their intent into a proper example/test file or explanatory comment.
Locate the commented block referencing GlobalResultCallback and
output.ResultEvent in the test (the lines starting with //
ne.GlobalResultCallback(...)) and remove it so the test file contains no
leftover commented-out code.
- Around line 93-96: The current pattern wraps require.NoError(t, err, ...)
inside an unnecessary if err != nil { ... } guard around calls to
nuclei.NewThreadSafeNucleiEngineCtx; remove the redundant if checks and directly
call require.NoError(t, err, "could not create nuclei engine") (and analogous
messages) after the NewThreadSafeNucleiEngineCtx calls so the test
short-circuits idiomatically; update all three occurrences that follow this
pattern (look for NewThreadSafeNucleiEngineCtx + err + require.NoError usage).
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
3 participants
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 fixes #6943. In the
createEphemeralObjects, the nil cast to the hosterrorscache.CacheInterface interface has been removed. This previously caused a panic when accessing data here since the interface was not null (but its value does).Proof
Previously, this code caused a panic when executing the ExecuteNucleiWithOptsCtx function, now the problem is fixed.
Checklist
Summary by CodeRabbit
Tests
Style