feat(security): defense-in-depth security analyzers#2472
feat(security): defense-in-depth security analyzers#2472csmith49 merged 21 commits intoOpenHands:mainfrom
Conversation
…al tests Single-file example implementing layered security analysis for agent tool calls: whitelisted extraction with field boundaries, Unicode normalization (NFKC + invisible/bidi stripping), segment-aware deterministic policy rails, regex pattern scanning, and max-severity ensemble fusion. Includes baseline test suite (93 tests) covering extraction, normalization, rails, pattern classification, ensemble fusion, and confirmation policy. Adversarial test suite (32 tests) with TDD-driven bug fixes, strict xfails documenting irreducible limitations, and hostile input stress tests. Closes OpenHands#2067
The 30k-char extraction cap silently drops content past the limit. Integrators who don't read the adversarial test suite won't know they're exposed if their agent processes large inputs.
The digit-prefixed example filename (45_...) requires importlib to import. Move the hack into conftest.py so it runs once at collection time; both test files now just reference sys.modules.
|
@OpenHands Do a /codereview on this PR. Investigate deeply the relevant code in the codebase. |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
I did a deeper pass on the example + tests and found two important correctness issues that look worth fixing before merge. Current targeted tests/examples pass, but both of the cases below reproduce locally with small ActionEvent probes.
[examples/01_standalone_sdk/45_defense_in_depth_security.py, Lines 153-161 and 563-577] 🔍 Correctness: _extract_segments() feeds thought, reasoning_content, and summary into the same corpus that shell-destructive rails/patterns scan. That means non-executable reasoning text can flip an otherwise safe action to HIGH. Repro: an action whose actual command is ls /tmp but whose thought says I should avoid rm -rf / currently returns HIGH in both PatternSecurityAnalyzer and EnsembleSecurityAnalyzer. That seems to undercut the stated goal of avoiding reasoning-related false positives. I think the safest fix is to keep shell/permission detection scoped to executable fields (tool_call.arguments, maybe tool metadata) and, if you still want thought/summary coverage, handle that in a separate prompt-injection/text-only path.
[examples/01_standalone_sdk/45_defense_in_depth_security.py, Lines 345-346 and 446] 🛠️ Correctness: the raw-disk dd detection only matches if=... of=/dev/.... dd operands are order-independent, so common destructive forms like dd of=/dev/sda if=/dev/zero and dd bs=1M of=/dev/sda if=/dev/zero currently fall through as LOW end-to-end (rail PASS, pattern LOW, ensemble LOW). Please make this check order-independent and add a regression test for of= before if=.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Thank you @enyst for the feedback. I am drafting fixes and will push again when they are ready. |
Finding 1: shell-destructive patterns now scan only executable fields (tool_name, tool_call.name, tool_call.arguments). Thought, reasoning, and summary are scanned only for injection/social-engineering patterns. Prevents false positives when an agent reasons about dangerous commands it chose not to run. Finding 2: dd raw-disk detection now matches of=/dev/ regardless of operand order. Adds regression tests for reversed and interleaved operands.
Co-authored-by: openhands <openhands@all-hands.dev>
|
Companion docs PR: OpenHands/docs#402 |
There was a problem hiding this comment.
Thanks for the contribution here! I'll leave some file-level comments/questions, but I also have a small project org. concern with the tests.
It's definitely a change in how things are organized to add SDK tests to things that are only in the examples. The current pattern is that the examples surface things in the SDK, so we don't need "example tests" because they're captured by the standard unit tests.
We could definitely just roll with this, but an alternative I prefer would be to move the relevant security analyzers to the SDK proper and just have the example surface how to use/configure them. They're general enough and useful enough that people building on the SDK might want to use them without the dynamic module loading tricks relied upon in the examples.
ETA: another alternative might be turning the example into a folder and package the tests directly with it instead of in tests/sdk.
@xingyaoww Any thoughts on the organization here?
|
@csmith49 Thank you Calvin for the substantive feedback. It seems the requested changes only make sense if this is moved into the SDK proper, so I will wait for feedback from @xingyaoww before pushing any new commits. |
Agree with this! We could potentially just come up with a new type of SecurityAnalyzer instead of dumping all the logic in the example script. I'd love to get this actually into SDK, maybe even as a security analyzer that always run since it doesn't seems to consume too much resource 🙏 |
|
@xingyaoww @csmith49 Solid direction, thank you. I'll refactor it into a new SecurityAnalyzer type in the SDK. Before I push anything, I'll study the existing analyzer module structure to make sure it fits the current patterns. Any preferences on where it should live under openhands_aci/? |
|
I expect most everything can live as a sub-module in |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @Fieldnote-Echo, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
1 similar comment
|
[Automatic Post]: It has been a while since there was any activity on this PR. @Fieldnote-Echo, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
Lead with the problem being solved, then the solution, then caveats. Module and class docstrings now explain what the reader needs to DO (use the analyzer) before explaining how it works internally. Docs guide restructured: problem -> what you get -> quick start -> composition -> design rationale -> limitations.
…-depth-sdk-promotion
Fieldnote-Echo
changed the title
|
@csmith49 All four review comments are addressed in the latest push. The analyzers now live in |
|
Just to note, the docs and review gate in CI are not mandatory, please don't be concerned about them - though the docs PR is most welcome of course! |
- _walk_json_strings: let RecursionError propagate to _extract_exec_segments raw-argument fallback instead of returning [] (Codex P1 — silent false-negative path for deeply nested payloads) - U+2028/U+2029: keep as whitespace for \s+ collapse instead of stripping (Codex P2 — stripping merges tokens, bypasses word-boundary regex detectors) - Downgrade per-match logs from info to debug to prevent log flooding in high-volume environments (Gemini — rate-limiting deferred)
|
@xingyaoww One rollout note for later discussion, not for this PR: if the team eventually wants broader/default use, I think the main follow-ups are observability of why something was flagged and some real-world false-positive tuning on coding workflows. I kept those out of this PR so the SDK promotion stays tight, but I’m happy to open follow-up issues once this lands. These are more product decisions requiring OpenHands judgement and deployment experience specifically. |
|
@enyst Okay team, I believe this and the docs companion PR are "review-ready" - Docs PR: OpenHands/docs#402 |
openhands-sdk/openhands/sdk/security/defense_in_depth/ensemble.py
Outdated
Show resolved
Hide resolved
openhands-sdk/openhands/sdk/security/defense_in_depth/policy_rails.py
Outdated
Show resolved
Hide resolved
|
Hey @Fieldnote-Echo, this is coming along great. I left some comments (mostly suggestions), once you get a chance to look through those and see if they're relevant we can get this merged. Excited to get onto the observability/tuning so we can make these default as well! |
- Move EnsembleSecurityAnalyzer to openhands.sdk.security.ensemble (top-level, composes any analyzers — not limited to defense-in-depth) - Add propagate_unknown: bool = False to ensemble (opt-in strict mode where any child UNKNOWN propagates immediately) - Simplify RailDecision: drop RailOutcome enum, outcome is now SecurityRisk directly (HIGH/LOW). reason field preserved. - Deduplicate SecurityRisk ordering via shared _RISK_ORDER dict and _check_comparable helper. Add __le__/__ge__ for complete surface. @total_ordering cannot help here due to str Enum MRO. - Update PolicyRailSecurityAnalyzer docstring: clarify rails are separate from patterns for interpretability, not regex capability - Delete accidental docs-draft-defense-in-depth.mdx 218 passed, 5 xfailed, 0 failures. Coding-Agent: claude-code Model: claude-opus-4-6
Document the new ensemble propagate_unknown flag in the stricter environments section and update the UNKNOWN explanation to cover both default and strict modes. Companion to OpenHands/software-agent-sdk#2472. Coding-Agent: claude-code Model: claude-opus-4-6
|
@csmith49 Thank you for the polish and suggestions! I left one convo open in case you'd like a rename for clarity. Other than that, everything else is resolved. |
csmith49
left a comment
csmith49
left a comment
There was a problem hiding this comment.
Much needed security analyzers and infrastructure to help fold them easily into our existing infra. Nice work!
5 participants
Summary
Promotes the defense-in-depth security analyzer from a standalone example into first-class SDK analyzers under
openhands.sdk.security.This lands a deterministic, local, composable security analyzer family in the SDK. It does not change default wiring or claim universal safety as a default-on control.
Public analyzers
PatternSecurityAnalyzer— regex-based threat detection with two-corpus scanning. Shell-destructive patterns scan executable fields only; injection patterns scan all fields including reasoning text. Lives inopenhands.sdk.security.defense_in_depth.PolicyRailSecurityAnalyzer— deterministic segment-aware rails for composed threats: fetch-to-exec, raw-disk-op, catastrophic-delete. Lives inopenhands.sdk.security.defense_in_depth.EnsembleSecurityAnalyzer— pure combiner with max-severity fusion. No detection logic of its own. UNKNOWN preserved as first-class. Lives atopenhands.sdk.security.ensemble(top-level, composes any analyzers — not limited to defense-in-depth siblings).All three are importable from
openhands.sdk.security.Key changes
SecurityRiskcomparison methods (__lt__,__gt__,__le__,__ge__) explicitly defined with shared_RISK_ORDERdict somax()works naturally on risk levels.@total_orderingcannot help here due tostr, EnumMRO — documented in code.EnsembleSecurityAnalyzermoved to top-levelopenhands.sdk.security.ensemble(per reviewer feedback — it composes analyzers generally, not just defense-in-depth siblings)propagate_unknown: bool = Falseadded to ensemble — default preserves current semantics;Truemakes any child UNKNOWN propagate immediately for stricter environmentsRailDecisionsimplified:RailOutcomeenum removed,outcomeis nowSecurityRiskdirectly (HIGH/LOW).reasonfield preserved for observability.enable_policy_rails, no extraction, no normalizationPolicyRailSecurityAnalyzer)DET_*+ 3RAIL_*)propagate_unknownsurvives round-trip)47_)conftest.pyimportlib hack deleteddocs-draft-defense-in-depth.mdxremovedWhat this does NOT do
ConversationState,VerificationSettings, or agent wiringTest results
218 passed, 5 strict xfailed, 0 failures. All pre-commit hooks pass (ruff, pyright, import rules).
Companion docs PR: OpenHands/docs#402
Test plan
uv run pytest tests/sdk/security/ -v— 218 pass, 5 xfailpropagate_unknown=True)propagate_unknownflag matrix covered)