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 4 commits 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>
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
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
enyst
left a comment
There was a problem hiding this comment.
Why did the LLM return any security risk, if no analyzer was configured?
This report documents: - Comprehensive testing of the fix (29/29 tests pass) - Manual integration testing confirming the fix works correctly - Architecture explanation of how security_risk extraction works - Troubleshooting guide for @XZ-X to verify the fix in their environment - Possible causes if still seeing 'low' instead of 'unknown' The fix is correct and thoroughly tested. If @XZ-X is still experiencing the issue, it's likely due to not using the code from this PR branch or environmental factors (plugins, caching, etc). Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ PR Artifacts Cleaned Up The |
Summary of InvestigationI thoroughly investigated PR #2130 in response to @XZ-X's report that they still see "security risk" as "low" instead of "unknown" despite the fix. What I Did
ConclusionThe fix is CORRECT and thoroughly tested. The implementation properly ignores LLM-provided security_risk values when no analyzer is configured. If @XZ-X is still seeing "low" instead of "unknown", it's due to environmental factors:
Changes MadeI added
This report has been committed and pushed to the PR branch for @XZ-X to review. No changes to the actual fix code were needed - the fix is correct as implemented. |
Ah, I think the answer is here:
OK 🤔 I believe Rohit had implemented this, if you need a second opinion, but SGTM. @juanmichelini |
API breakage checks (Griffe)Result: Passed |
|
@openhands-agent I still see from openhands.sdk import (
LLM,
Agent,
Conversation,
Event,
Message,
TextContent,
)
from openhands.sdk.logger import setup_logging
from openhands.tools.preset.default import get_default_tools
from openhands.workspace import DockerWorkspace
workspace = DockerWorkspace(
# base_image="nikolaik/python-nodejs:python3.12-nodejs22",
server_image="ghcr.io/openhands/agent-server:latest-python",
working_dir="/workspace",
detach_logs=False,
)
task = open("danger.txt").read()
prompt = """
You are a helpful assistant that can help with coding tasks
Here is the task:
{task}
""".format(
task=task
)
msg = Message(
role="user",
content=[TextContent(text=prompt)],
)
llm = LLM(
model="Qwen3-Coder-30B-A3B",
base_url="....",
api_key="...",
custom_llm_provider="openai",
)
system_prompt_kwargs: dict[str, object] = {"cli_mode": False, "llm_security_analyzer": False}
agent = Agent(
llm=llm,
tools=get_default_tools(enable_browser=False),
system_prompt_kwargs=system_prompt_kwargs,
)
conversation = Conversation(
agent=agent,
workspace=workspace,
# callbacks=[lambda ev: logger.info("Event: %s", ev)],
callbacks=[],
max_iteration_per_run=3,
)
conversation.send_message(msg)
try:
conversation.run()
except Exception as e:
# should be exceed max step error, it's okay.
pass
# import traceback
# traceback.print_exc()
events: list[Event] = list(conversation.state.events)
for event in events:
if hasattr(event, "security_risk"):
print(event.security_risk)
workspace.cleanup()And I validated my repo commit id as follows: % git rev-parse HEAD
e988c6aa0470038bee45878f1c1d1d2211c08167I also execute |
|
@OpenHands investigate and fix #2130 (comment) |
|
I'm on it! enyst can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
Summary (new work since last update)I investigated the PR comment report (“still seeing To make this easy to verify (and to unblock debugging), I implemented and pushed agent-server build/version introspection:
Checklist
Git / PR update
This enables the reporter to verify whether their running |
|
Oh, I see. @juanmichelini and @XZ-X , please feel free to use or remove the agent's last commit, as you see fit. It just tries to show info in docker |
5 participants
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:16daf37-pythonRun
All tags pushed for this build
About Multi-Architecture Support
16daf37-python) is a multi-arch manifest supporting both amd64 and arm6416daf37-python-amd64) are also available if needed