add maxtime option to limit entire execution time #6600
Conversation
WalkthroughImplements a maximum execution time feature by adding a new CLI flag Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI Entry
participant Runner
participant TimeoutCtx as Timeout Context
participant ScanService
participant Engine
User->>CLI: Start nuclei with -mt 1h
CLI->>Runner: Execute with Options.MaxTime set
Runner->>TimeoutCtx: ApplyMaxTimeContext(ctx, opts)
alt MaxTime > 0 and no existing deadline
TimeoutCtx-->>Runner: (new_ctx, cancel_func, true)
Note over TimeoutCtx: Context with timeout<br/>configured
else
TimeoutCtx-->>Runner: (original_ctx, nil, false)
end
Runner->>ScanService: NewWithContext(opts, timeout_ctx)
ScanService->>ScanService: Store ctx, cancel func
Runner->>Engine: ExecuteScanWithOpts(timeout_ctx)
par Normal Completion
Engine->>Engine: Execute scan
Engine-->>Runner: Scan complete
Runner->>ScanService: Close() → cancel()
and Timeout Reached
TimeoutCtx->>Engine: timeout triggers context.Done()
Engine-->>Runner: ctx.Err() = context.DeadlineExceeded
Note over TimeoutCtx: Log timeout<br/>Graceful stop
Runner->>ScanService: Close() → cancel()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 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. |
Proposed changes
closes #5823
Checklist
Summary by CodeRabbit
Release Notes
New Features
--max-time(or-mt) CLI flag to control maximum execution duration for scans. When the time limit is reached, scans gracefully terminate and complete their operations. Supports flexible duration formats (e.g.,1h,30m,1h30m).Documentation