Skip to content

docs: deep production audit 2026-05-07 (cross-repo) - #6

Draft
diskhacker wants to merge 1 commit into
mainfrom
claude/create-main-branch-audit-SsbNG
Draft

docs: deep production audit 2026-05-07 (cross-repo)#6
diskhacker wants to merge 1 commit into
mainfrom
claude/create-main-branch-audit-SsbNG

Conversation

@diskhacker

Copy link
Copy Markdown
Owner

Summary

Adds DEEP-AUDIT-REPORT-2026-05-07.md — cross-repo deep audit. Same file committed to claude/create-main-branch-audit-SsbNG in all 7 repos.

sigops-agent-specific P0 / P1

  • P0: SecurityPolicy is dead code on the hot path. src/executor.rs invokes SysCommand::new(...) directly for sigops.restart, sigops.http, sigops.notify_slack, sigops.condition, sigops.wait — bypassing the whitelist, deny-list, timeout, and ..-traversal checks defined in src/security.rs. The 24 hardening tests pass but no production caller wires policy.run(). CLAUDE.md HARD RULE 3 ("No shell injection — all commands go through the whitelist") is therefore not enforced.
  • P0: sigops.restart accepts attacker-controlled cmd.input["method"] with no allow-list. A malicious server can set method to any binary on PATH; service-name sanitization protects only the second arg.
  • P1: Health server in src/health.rs binds 0.0.0.0:9100 (public on every NIC) and doesn't parse the request line — every TCP connect returns agent metadata regardless of method/path. Information disclosure.
  • P1: README mentions --no-health and SIGOPS_HEALTH_PORT but Config has neither field; port hardcoded in main.rs.
  • P1: tracing::warn!(body = %body) in src/heartbeat.rs error paths logs full server response body — may leak rotated-token JSON on error responses.
  • P1: README env-var names (SIGOPS_SERVER_URL, SIGOPS_API_KEY, SIGOPS_POLL_INTERVAL, SIGOPS_HEALTH_PORT, SIGOPS_NO_HEALTH, SIGOPS_LOG_LEVEL) don't match clap (API_TOKEN, HEARTBEAT_INTERVAL, etc.) — operators following README will fail to configure.
  • P2: Config::api_token defaults to empty string — agent will start with no auth.
  • P2: tokens.maybe_rotate accepts arbitrary server-issued tokens; default TokenStore::new doesn't persist → rotation lost on restart.
  • P2: sigops.notify_slack skips validate_url for webhookUrl.
  • P2: executor::execute_wait uses blocking std::thread::sleep inside tokio runtime — up to 3600s blocks calling thread.
  • P3: Graceful shutdown does not cancel in-flight executor tasks (long sigops.wait outlives shutdown).

Test plan

  • Review report
  • Highest priority: route every built-in tool through policy.run() and add method allow-list for sigops.restart (P0 — single PR closes the largest open security gap in the ecosystem)
  • Health server: parse HTTP request line; return 404 for unknown paths; default-bind 127.0.0.1
  • Wire --no-health / --health-port to Config; sync README env-var names with clap

Draft for visibility only — no code changes.


Generated by Claude Code

@coderabbitai

coderabbitai Bot commented May 7, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: adce8f51-d0b7-417c-bd61-c87a596f5024

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/create-main-branch-audit-SsbNG

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Owner Author

CI note: the three Test (*) failures (ubuntu/macos/windows) are pre-existing on main, not caused by this change.

This PR adds a single file (DEEP-AUDIT-REPORT-2026-05-07.md) at the repo root — no Rust, Cargo, or workflow changes. A markdown-only diff cannot break cargo fmt, cargo clippy -D warnings, or cargo test --all-targets --all-features.

main HEAD is 516dfc9 (2026-04-21, 2+ weeks ago) and CI was green when that commit landed. No PR has run CI against main since, so this is the first time we're seeing post-landing breakage — most likely a dtolnay/rust-toolchain@stable bump (new Rust version with stricter clippy lints) or a runner-image change. It's also possible a transitive dep changed semver-compatibly.

The audit report itself flags the SecurityPolicy dead-code P0 in executor.rs and several other items; the CI regression here is operational and worth investigating separately. I'm not pushing speculative fixes from this PR.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant