Validate tuning inputs; remove dead pass block (audit fixes #989)
- _prompt() clamps non-positive tuning values to default with operator warning (HIGH from AUDIT_SEC_989: prevents garbage display when vars file has masscan_rate: -100 or nuclei_concurrency: 0) - Remove empty 'if scan_mode in (...): pass' placeholder left from refactor (P3 from AUDIT_CODE_989) - Add AUDIT_CODE/SEC/ENV_989.md and FIXES_989.md with skip justifications (path traversal not applicable to single-operator threat model; Tor wording already adequate; estimate disclosure interactive-only)
This commit is contained in:
n0mad1k
@@ -0,0 +1,88 @@
|
||||
---
|
||||
agent: code-auditor
|
||||
status: COMPLETE
|
||||
timestamp: 2026-05-02T00:00:00Z
|
||||
duration_seconds: 180
|
||||
files_scanned: 2
|
||||
findings_count: 5
|
||||
errors: []
|
||||
skipped_checks: []
|
||||
---
|
||||
|
||||
# Code Audit — DevTrack #989: Webrunner Estimator Rewrite
|
||||
|
||||
## Findings
|
||||
|
||||
### P2 (Medium Priority)
|
||||
|
||||
- **`modules/webrunner/deploy_webrunner.py:306`** — `gather_webrunner_parameters()` is 227 lines. Complexity is high due to sequential parameter gathering, but each section is self-contained (providers, countries, scan mode, tuning, estimate display, preset selection, CIDR distribution). Refactor would require breaking into multiple functions that each prompt and return data — tradeoff: current structure is arguably clearer for a linear setup wizard. **Recommendation**: Document the phases with section headers (already done with comments) and consider splitting only if interactivity requirements change.
|
||||
|
||||
- **`utils/provider_rates.py:87-145`** — `estimate_scan_hours()` has cyclomatic complexity of 11 (threshold: 10). The complexity is necessary: 4 scan modes × nested `use_tor` conditions = 5 paths per phase × 4 phases + rate check. Each branch is distinct (different time calculations). Logic is correct but at the boundary. **Recommendation**: acceptable due to inherent problem structure, but monitor for future growth.
|
||||
|
||||
- **`modules/webrunner/deploy_webrunner.py:200,206`** — Tuning params `nmap_timeout` and `nuclei_timeout` are prompted from user (lines 200, 206) and stored in config (lines 559, 562 show them printed), but **NOT passed to `estimate_scan_hours()`** in provider_rates.py. These parameters **DO NOT affect the time estimate** — they are only used at scan execution time (node_scanner.py lines 376-381, 396). This is **correct behavior** but the params are dead-code for estimation purposes. Current design: `estimate_scan_hours()` uses fixed constants (`NMAP_TIME_BY_TIMING`, `NUCLEI_TIME_PER_TARGET_SEC`) which are conservative defaults. The tuning params would require significant refactor to propagate through the estimation function signature (nmap_timeout not indexed by timing template, nuclei_timeout not indexed by template complexity). **Assessment**: No bug, by design. Timeouts are runtime constraints, not estimation inputs.
|
||||
|
||||
### P3 (Low Priority)
|
||||
|
||||
- **`modules/webrunner/deploy_webrunner.py:196`** — Dead `pass` statement. Line 195-196:
|
||||
```python
|
||||
if scan_mode in ('masscan+nmap', 'geo-scout', 'masscan+nuclei'):
|
||||
pass # masscan_rate already handled
|
||||
```
|
||||
This is a placeholder comment; the condition is never needed since `masscan_rate` is always prompted on line 193. **Recommendation**: Remove the conditional block entirely.
|
||||
|
||||
- **`modules/webrunner/deploy_webrunner.py:265,279,281`** — Magic numbers (70, 56, 10, 25) in format strings are visual constants for table borders and tuning display. All acceptable.
|
||||
|
||||
### P4 (Informational)
|
||||
|
||||
- No `print()` statements in production code detected — all output uses Rich-style ANSI color codes via `COLORS` dict (good practice for TUI).
|
||||
- No TODO/FIXME/HACK comments detected.
|
||||
- No unused imports detected.
|
||||
- All function signatures updated consistently (`_show_estimate_table` receives tuning dict, passes to `build_estimate_table`).
|
||||
- Nuclei phase throughput calculation at `utils/provider_rates.py:140-142` is mathematically correct:
|
||||
```
|
||||
parallel_throughput = nuclei_concurrency / per_target
|
||||
rate_throughput = nuclei_rate
|
||||
effective_rps = min(rate_throughput, parallel_throughput)
|
||||
```
|
||||
With defaults (25 concurrency, 5s per target, 150 req/s rate): min(150, 5) = 5 req/s bottleneck (parallelism-limited). Edge cases handled correctly.
|
||||
|
||||
## Stats
|
||||
|
||||
- **Files >500 lines**: 1 (`deploy_webrunner.py`, 617 lines)
|
||||
- **Functions >50 lines**: 1 (`gather_webrunner_parameters`, 227 lines)
|
||||
- **Cyclomatic complexity >10**: 1 (`estimate_scan_hours`, CC=11)
|
||||
- **Nesting depth >3**: 1 instance (L60, necessary for loop logic)
|
||||
- **print() calls**: 0 (ANSI output via COLORS dict only)
|
||||
- **TODO/FIXME/HACK**: 0
|
||||
- **Dead code blocks**: 1 placeholder conditional (lines 195-196)
|
||||
- **Unused imports**: 0
|
||||
- **Signature changes**: 1 (`_show_estimate_table` + tuning param) — validated across 2 call sites
|
||||
|
||||
## Verification Notes
|
||||
|
||||
### Nuclei Throughput Logic ✓
|
||||
The calculation `effective_rps = min(rate_throughput, parallel_throughput)` correctly identifies which constraint is tighter:
|
||||
- If concurrency is small relative to per_target time (e.g., 25 / 5 = 5 req/s), parallelism is the bottleneck.
|
||||
- If rate limit is small (e.g., 2 req/s), rate limit is the bottleneck.
|
||||
- The `max(min(...), 1.0)` ensures effective_rps ≥ 1.0 (prevents division by very small numbers).
|
||||
|
||||
### Phase Decomposition Correctness ✓
|
||||
Each phase (masscan, nmap, probe, nuclei) applies Tor multiplier only to TCP-based operations (nmap, nuclei, probe), never to masscan (raw sockets). This is correct per OPSEC comment on line 77-79.
|
||||
|
||||
### Tuning Param Integration ✓
|
||||
All 6 tuning params extracted in `build_estimate_table()` are correctly passed to `estimate_scan_hours()`:
|
||||
- `nmap_timing`, `nmap_workers`, `nuclei_rate`, `nuclei_concurrency`, `hit_rate`, `masscan_rate`
|
||||
- No params are lost; no params are unused.
|
||||
|
||||
---
|
||||
|
||||
## No Code Quality Issues Detected
|
||||
|
||||
All critical paths reviewed:
|
||||
- Module imports: complete and used
|
||||
- Function signatures: refactored correctly
|
||||
- Logic bugs: none detected
|
||||
- Dead code: 1 placeholder conditional (recommended removal)
|
||||
- Complexity: at acceptable threshold, necessary for problem domain
|
||||
|
||||
**Refactor Status**: Ready for deployment. Consider future simplification of `gather_webrunner_parameters()` if user interaction patterns change.
|
||||
Reference in New Issue
Block a user