add maxtime option to limit entire execution time#6600
add maxtime option to limit entire execution time#6600dogancanbakir wants to merge 2 commits intodevfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR adds registration of common CLI flags by calling Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use your project's `golangci-lint` configuration to improve the quality of Go code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/types/context.go (1)
12-30: Consider goroutine lifecycle.The goroutine spawned on lines 22-27 will remain blocked on
execCtx.Done()until the timeout expires or the context is cancelled. This is generally acceptable for a logging goroutine, but be aware that:
- If the scan completes before the timeout, the goroutine will continue running until timeout or cancellation
- The goroutine holds a reference to
execCtxandlogger, preventing GC until it completesThis is a common pattern for timeout logging and should be fine for the use case, but consider documenting this behavior.
internal/runner/runner.go (1)
796-807: Verify if redundant max-time context application is intentional.The max-time context is applied here at the runner level (lines 797), but
automaticscan.NewWithContextalso applies it internally. While safe due to thehasDeadlineguard inApplyMaxTimeContext, this redundancy creates ambiguity about ownership and could confuse future maintainers.Consider clarifying the design:
- Option 1 (recommended): Apply max-time context only at the runner level and remove the internal application from
automaticscan.NewWithContext- Option 2: Remove the application here and let
automaticscan.NewWithContexthandle it exclusivelyOption 1 is preferred for centralized control and consistency with
executeTemplatesInput(which must apply the context sinceExecuteScanWithOptsdoesn't).Based on relevant code snippets from
pkg/protocols/common/automaticscan/automaticscan.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)cmd/nuclei/main.go(1 hunks)internal/runner/runner.go(2 hunks)internal/server/nuclei_sdk.go(1 hunks)lib/multi.go(1 hunks)lib/sdk.go(1 hunks)pkg/protocols/common/automaticscan/automaticscan.go(5 hunks)pkg/types/context.go(1 hunks)pkg/types/types.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/types/types.golib/multi.gointernal/server/nuclei_sdk.golib/sdk.gocmd/nuclei/main.gopkg/types/context.gopkg/protocols/common/automaticscan/automaticscan.gointernal/runner/runner.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()
Files:
pkg/protocols/common/automaticscan/automaticscan.go
🧬 Code graph analysis (6)
lib/multi.go (1)
pkg/types/context.go (1)
ApplyMaxTimeContext(12-30)
internal/server/nuclei_sdk.go (2)
pkg/types/context.go (1)
ApplyMaxTimeContext(12-30)pkg/types/types.go (1)
Options(34-474)
lib/sdk.go (1)
pkg/types/context.go (1)
ApplyMaxTimeContext(12-30)
pkg/types/context.go (2)
pkg/protocols/common/contextargs/contextargs.go (1)
Context(22-33)pkg/types/types.go (1)
Options(34-474)
pkg/protocols/common/automaticscan/automaticscan.go (4)
pkg/protocols/common/contextargs/contextargs.go (2)
Context(22-33)New(36-38)pkg/types/types.go (1)
Options(34-474)pkg/types/context.go (1)
ApplyMaxTimeContext(12-30)pkg/input/provider/simple.go (1)
NewSimpleInputProviderWithUrls(22-28)
internal/runner/runner.go (3)
pkg/types/context.go (1)
ApplyMaxTimeContext(12-30)pkg/protocols/common/automaticscan/automaticscan.go (2)
NewWithContext(78-137)Options(47-52)pkg/types/types.go (1)
Options(34-474)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (13)
README.md (1)
302-302: LGTM!The documentation clearly describes the new max-time option with an appropriate example.
pkg/types/types.go (2)
316-317: LGTM!The MaxTime field addition is straightforward and properly documented.
615-615: LGTM!The Copy method correctly propagates the MaxTime field to the new instance.
internal/server/nuclei_sdk.go (1)
184-192: Verify context source.The code creates a fresh
context.Background()each time. Consider if there's an existing context (e.g., from the HTTP request or the nucleiExecutor struct) that should be used instead to propagate cancellation signals from upstream callers.cmd/nuclei/main.go (1)
433-433: LGTM!The CLI flag is correctly defined with appropriate names, type, default value, and description. The example in the description helps users understand the expected format.
lib/multi.go (1)
168-173: LGTM!The max-time context is correctly applied and the derived context is used for scan execution. The cancel function is properly deferred for cleanup.
lib/sdk.go (1)
268-284: LGTM!The max-time context is correctly applied and consistently used throughout the method. The completion/timeout handling in the select statement properly uses
execCtx.Done()andexecCtx.Err().pkg/protocols/common/automaticscan/automaticscan.go (4)
73-85: LGTM!The context-aware constructor is well-implemented:
- Properly defaults to
context.Background()if nil- Applies max-time context when options are available
- Stores the cancel function for lifecycle management
141-143: LGTM!The Close method correctly invokes the cancel function for proper context cleanup.
156-160: LGTM!The context cancellation check enables graceful early termination when the max-time is reached.
217-217: LGTM!Using
s.ctxinstead ofcontext.Background()correctly propagates the max-time context through the execution path.internal/runner/runner.go (2)
843-849: Correct implementation of max-time context for template execution.The context handling pattern is properly implemented:
- Context is created and bounded with max-time
- Cancel function is deferred with nil check to prevent panic
- Context is correctly passed to
ExecuteScanWithOptsThis is necessary at the runner level since the engine doesn't apply max-time internally.
796-807: Consider adding tests and documentation for max-time feature.The PR checklist indicates that tests and documentation haven't been added yet. Given that this is a new user-facing feature (
-mt/--max-timeflag), please ensure:
Tests: Add unit tests covering:
- Context timeout triggers graceful shutdown
- Cancel function is properly called
- Behavior when
MaxTime <= 0(feature disabled)- Behavior when context already has a deadline
Documentation: Update relevant docs to explain:
- The
-mt/--max-timeflag usage and format- Expected behavior when timeout is reached
- Impact on scan results (graceful stop vs hard kill)
Per PR objectives and coding guidelines.
Also applies to: 843-849
Mzack9999
left a comment
Mzack9999
left a comment
There was a problem hiding this comment.
I think the functionality can be already obtained by using nuclei as SDK and providing a context with max deadline execution for example. What do you think?
@Mzack9999, however, this is aimed explicitly at CLI users, as it won't be possible for them. |
Mzack9999
left a comment
Mzack9999
left a comment
There was a problem hiding this comment.
This request already came in for subfinder. System tools like timeout already allow control of global timing, but unfortunately not in a graceful way. I'd suggest a different strategy to plug the functionality transparently in all the tools, what do you think of adding something like CommonFlags to goflags so that in all tools it's enough to add one-line to the flagset with something like:
var opts Options
flagSet := goflags.NewFlagSet()
flagSet.AddCommonFlags(&opts)
Then we can simply send a system OS kill from the goflags library, that will be intercepted and handled by the grafecul exit already embedded in the tools.
Would this make sense?
dogancanbakir
force-pushed
the
5823_add_maxtime_option
branch
from
7f51789 to
bb112b6
Compare
2 participants
Proposed changes
closes #5823
Checklist
Summary by CodeRabbit