Add audit reports for #980 webrunner nmap parallelization

audits/FIXES_980.md

@@ -0,0 +1,149 @@
+---
+agent: fix-planner
+status: COMPLETE
+timestamp: 2026-05-01T23:45:00Z
+total_findings_raw: 15
+total_findings_deduped: 13
+p1_count: 1
+p2_count: 2
+p3_count: 3
+p4_count: 2
+devtrack_items_created: []
+errors: []
+---
+
+# Fix Plan — DevTrack #980: node_scanner.py ThreadPoolExecutor Parallelization
+
+## Summary
+
+Consolidated findings from code-auditor (8 findings) and security-auditor (2 findings) for DevTrack #980.
+
+**Status**: 7 fixes applied and verified. 2 HIGH/MEDIUM severity issues remain (run_scan.yml, estimate_scan_hours partial chunk logic). 4 low-priority items acceptable for backlog.
+
+---
+
+## Fixes Applied ✓
+
+### Applied Fixes (Verified)
+
+1. **[node_scanner.py:204-216]** ThreadPoolExecutor parallelization in `scan_masscan_nmap()` — 10 workers via `NMAP_WORKERS` env var (default 10)
+2. **[node_scanner.py:260-266]** ThreadPoolExecutor parallelization in `scan_geo_scout()` — same 10-worker pattern
+3. **[node_scanner.py:126]** Bytes format bug fixed: `ip.encode()` used directly instead of `%b` format string
+4. **[node_scanner.py:145-192]** Dead variable `targets_arg` removed from `scan_nmap_only()`
+5. **[provider_rates.py:55-63]** `estimate_scan_hours()` now accepts `scan_mode` parameter for accurate nmap time estimation
+6. **[provider_rates.py:50-52]** Constants added: `NMAP_TIME_PER_HOST_SEC=10`, `MASSCAN_HIT_RATE=0.01`, `_NMAP_WORKERS=10`
+7. **[provider_rates.py:104]** `build_estimate_table()` passes `scan_mode` to `estimate_scan_hours()`
+
+---
+
+## Remaining Issues
+
+### P1 — Block Deploy
+
+- [ ] **[HIGH] [run_scan.yml:14-18]** Ansible template injection via unquoted variables | Sources: security-auditor | Effort: S | Status: NOT FIXED
+
+  **Description**: Arguments in `command:` module use unquoted Ansible template vars (`{{ scan_mode }}`, `{{ ports_str }}`, `{{ node_name }}`). If `node_name` is injected with shell metacharacters (e.g., `node-1; rm -rf /`), Ansible's `command:` module will execute shell commands.
+
+  **Attack vector**: Operator misconfigures Ansible extra-vars with malicious node_name or ports_str → arbitrary command execution on cloud node.
+
+  **Remediation**: Use Ansible `command:` as list form (args: [python3, script.py, --mode, "{{ scan_mode }}", ...]) or validate inputs upstream in deploy_webrunner.py.
+
+  **Risk**: HIGH — affects production cloud deployment pipeline. Requires manual Ansible remediation (file not in Python audit scope).
+
+---
+
+### P2 — Fix This Week
+
+- [ ] **[HIGH] [provider_rates.py:91-118]** Partial chunk cost overestimation in `build_estimate_table()` | Sources: code-auditor | Effort: M | Status: NOT FIXED
+
+  **Description**: Line 104 assumes each chunk costs the same time as `preset['chunk_size']` IPs. If `total_ips % preset['chunk_size'] != 0`, the last chunk is smaller, but cost calculation doesn't account for it. Example: 2.5M IPs across 2-chunk preset (2M chunks) = chunk 1 (2M hrs) + chunk 2 (0.5M hrs), but current code bills chunk 2 as 2M hrs.
+
+  **Current code**: `hours = estimate_scan_hours(preset['chunk_size'], n_ports, mode_rate, scan_mode)` — always uses full chunk_size.
+
+  **Fix**: Calculate per-chunk IP count separately:
+  ```python
+  for i in range(n_chunks):
+      chunk_ips = min(preset['chunk_size'], total_ips - i * preset['chunk_size'])
+      hours = estimate_scan_hours(chunk_ips, n_ports, mode_rate, scan_mode)
+      provider = providers[i % len(providers)]
+      instance = DEFAULT_INSTANCE.get(provider, 'g6-standard-2')
+      total_cost += node_cost(provider, instance, hours)
+  ```
+
+  **Impact**: Cost estimates 1.5-2x too high for non-aligned IP counts; may oversell capacity planning.
+
+- [ ] **[MEDIUM] [node_scanner.py:225-239]** Import inside function — `import yaml` at line 232 | Sources: code-auditor | Effort: XS | Status: NOT FIXED
+
+  **Description**: `import yaml` appears inside `scan_geo_scout()` function. Move to top-level imports for clarity and consistency.
+
+  **Fix**: Add `import yaml` to line 16 (after `ET` import), remove line 232.
+
+---
+
+### P3 — Fix This Month
+
+- [ ] **[MEDIUM] [node_scanner.py:21-23]** `print()` in production code — `log()` function uses `print()` | Sources: code-auditor | Effort: S | Status: NOT FIXED
+
+  **Description**: Line 23 calls `print()` directly. Should use Python `logging` module or Rich (per c2itall style guide).
+
+  **Fix**: Replace `log()` function with logging.info or Rich console output for consistency.
+
+- [ ] **[LOW] [node_scanner.py:139-192]** Unused parameter `node_name` — passed to all scan_* functions but never used | Sources: code-auditor | Effort: XS | Status: NOT FIXED
+
+  **Description**: Parameter `node_name` appears in `scan_masscan_only()`, `scan_nmap_only()`, `scan_masscan_nmap()`, `scan_geo_scout()` signatures but is not referenced in function bodies. These functions don't write node metadata to results.
+
+  **Assessment**: Intentionally kept for interface consistency (caller always passes it, avoids conditional logic).
+
+  **Fix**: Document as "reserved for future node metadata logging" or remove for clarity. Non-blocking.
+
+- [ ] **[LOW] [provider_rates.py:76-88]** Comment ambiguity — Tor multiplier description misleading | Sources: code-auditor | Effort: XS | Status: NOT FIXED
+
+  **Description**: Line 86 comment says "Tor adds ~5x latency overhead" but line 88 sets `TOR_RATE_MULTIPLIER = 0.2` (which is 1/5, not 5x). Comment is correct in spirit but confusingly worded.
+
+  **Fix**: Change comment to "Tor reduces throughput to 1/5 (0.2x)" or "multiplier 0.2 ≈ 5x latency".
+
+---
+
+### P4 — Backlog
+
+- [ ] **[LOW] [provider_rates.py:57]** Magic number 0.018 — Linode default rate hardcoded | Status: NOT FIXED
+
+  **Description**: Line 7 (`'g6-standard-2': 0.018`) and line 67 (fallback rate) hardcode 0.018. Should be named constant.
+
+  **Fix**: Add `DEFAULT_INSTANCE_RATE = 0.018` and reference it.
+
+- [ ] **[LOW] [node_scanner.py]** Missing docstrings — module + function docstrings absent | Status: NOT FIXED
+
+  **Description**: Module docstring present (line 2-4) but individual functions lack docstrings. Low impact — code is self-documenting.
+
+---
+
+## Deduplication Notes
+
+- **Socket leak risk (probe_service)**: Marked as false alarm in code-auditor; context manager guarantees cleanup. No action needed.
+- **Thread safety (run_nmap concurrent writes)**: Verified safe — per-IP XML files unique (`nmap_{ip.replace('.', '_')}.xml`). No collision risk.
+- **XXE/entity expansion in ET.parse()**: PASS — ET default config disables external entity expansion. nmap output is trusted. No action needed.
+- **Resource exhaustion (socket connections)**: MEDIUM severity in security-auditor. Noted as acceptable risk for current sequential structure. ThreadPoolExecutor 10-worker design with 3-second socket timeout is within system limits. Semaphore capping recommended for future scaling but not blocking.
+
+---
+
+## Action Items
+
+### Critical Path (Block #980 deployment):
+1. **run_scan.yml**: Convert Ansible `command:` module to list form OR validate node_name upstream
+2. **provider_rates.py**: Fix partial-chunk cost calculation in `build_estimate_table()`
+
+### Nice-to-have (P3-P4):
+- Move `import yaml` to top level
+- Replace `print()` with logging/Rich
+- Add Tor multiplier comment clarity
+- Extract 0.018 magic number to constant
+
+---
+
+## Test Coverage
+
+- Unit tests for `estimate_scan_hours()` with partial chunks should verify cost accuracy
+- Ansible playbook syntax validation required for run_scan.yml
+- Manual integration test with non-aligned IP count (e.g., 2.5M across 2M presets)
+