Fix: Security analyzer ignores LLM security_risk when no analyzer is configured#2130
Fix: Security analyzer ignores LLM security_risk when no analyzer is configured#2130juanmichelini wants to merge 1 commit intomainfrom
Conversation
… set Previously, when llm_security_analyzer=False (or when no security analyzer was configured), the agent would still evaluate and use the security_risk value provided by the LLM. This caused actions to be incorrectly flagged with security risks even when security analysis was disabled. This fix modifies _extract_security_risk() to return SecurityRisk.UNKNOWN when security_analyzer is None, effectively ignoring any security_risk value provided by the LLM when no analyzer is configured. The schema still includes security_risk field (via add_security_risk_prediction=True) for consistency, but the field is now properly ignored at runtime when no analyzer is set. Fixes #1957 Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean, pragmatic bug fix. The early return in _extract_security_risk() properly separates "schema includes field" from "runtime uses field value". Tests reproduce the actual bug from #1957 and verify the fix. LGTM!
|
@OpenHands @XZ-X has tested it but still sees “security risk” as “low”, not “unknown” as expected from this PR. Please double check the tests are testing correctly, and rerun them. Also do a manual integration test to first reproduce the issue, then make sure the fix addresses the issue. If not reconsider it. |
|
I'm on it! juanmichelini can track my progress at all-hands.dev |
enyst
left a comment
There was a problem hiding this comment.
Why did the LLM return any security risk, if no analyzer was configured?
Summary
Fixes #1957
Previously, when
llm_security_analyzer=False(or when no security analyzer was configured viaconversation.set_security_analyzer()), the agent would still evaluate and use thesecurity_riskvalue provided by the LLM. This caused actions to be incorrectly flagged with security risks even when security analysis was explicitly disabled.Root Cause
The
_extract_security_risk()method inagent.pywas extracting and using the LLM-providedsecurity_riskvalue regardless of whether a security analyzer was configured. The code only checked if the analyzer was an instance ofLLMSecurityAnalyzerbut didn't handle the case whensecurity_analyzer is None.Solution
Modified
_extract_security_risk()to returnSecurityRisk.UNKNOWNwhensecurity_analyzer is None, effectively ignoring anysecurity_riskvalue provided by the LLM when no analyzer is configured.Why
add_security_risk_prediction=Trueis NOT changedThe
add_security_risk_prediction=Trueparameter inopenhands/sdk/agent/utils.pyis intentionally kept unchanged. This parameter ensures thesecurity_riskfield is always included in tool schemas for consistency, even when the security analyzer is disabled. The schema consistency helps:_extract_security_risk(), not the schemaThe fix ensures that the field is properly ignored at runtime when no analyzer is set, which is the correct behavior as documented in the code comments.
Changes
openhands-sdk/openhands/sdk/agent/agent.py: Added early return in_extract_security_risk()to returnUNKNOWNwhensecurity_analyzer is Nonetests/sdk/agent/test_extract_security_risk.py: Updated test expectations to verify thatsecurity_riskis ignored (returnsUNKNOWN) when no analyzer is settests/sdk/agent/test_security_policy_integration.py: Enhanced existing test to verify the fix for the reported issueChecklist
@juanmichelini can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:457a5b6-pythonRun
All tags pushed for this build
About Multi-Architecture Support
457a5b6-python) is a multi-arch manifest supporting both amd64 and arm64457a5b6-python-amd64) are also available if needed