mirror of
https://github.com/All-Hands-AI/OpenHands.git
synced 2026-01-09 14:57:59 -05:00
feat: support security analyzer settings for v1 conversations (#12008)
This commit is contained in:
Hiep Le
@@ -4,7 +4,11 @@ import tempfile
|
||||
from abc import ABC
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
from typing import AsyncGenerator
|
||||
from typing import TYPE_CHECKING, AsyncGenerator
|
||||
from uuid import UUID
|
||||
|
||||
if TYPE_CHECKING:
|
||||
import httpx
|
||||
|
||||
import base62
|
||||
|
||||
@@ -29,6 +33,14 @@ from openhands.sdk.context.agent_context import AgentContext
|
||||
from openhands.sdk.context.condenser import LLMSummarizingCondenser
|
||||
from openhands.sdk.context.skills import load_user_skills
|
||||
from openhands.sdk.llm import LLM
|
||||
from openhands.sdk.security.analyzer import SecurityAnalyzerBase
|
||||
from openhands.sdk.security.confirmation_policy import (
|
||||
AlwaysConfirm,
|
||||
ConfirmationPolicyBase,
|
||||
ConfirmRisky,
|
||||
NeverConfirm,
|
||||
)
|
||||
from openhands.sdk.security.llm_analyzer import LLMSecurityAnalyzer
|
||||
from openhands.sdk.workspace.remote.async_remote_workspace import AsyncRemoteWorkspace
|
||||
|
||||
_logger = logging.getLogger(__name__)
|
||||
@@ -379,3 +391,95 @@ class AppConversationServiceBase(AppConversationService, ABC):
|
||||
condenser = LLMSummarizingCondenser(**condenser_kwargs)
|
||||
|
||||
return condenser
|
||||
|
||||
def _create_security_analyzer_from_string(
|
||||
self, security_analyzer_str: str | None
|
||||
) -> SecurityAnalyzerBase | None:
|
||||
"""Convert security analyzer string from settings to SecurityAnalyzerBase instance.
|
||||
|
||||
Args:
|
||||
security_analyzer_str: String value from settings. Valid values:
|
||||
- "llm" -> LLMSecurityAnalyzer
|
||||
- "none" or None -> None
|
||||
- Other values -> None (unsupported analyzers are ignored)
|
||||
|
||||
Returns:
|
||||
SecurityAnalyzerBase instance or None
|
||||
"""
|
||||
if not security_analyzer_str or security_analyzer_str.lower() == 'none':
|
||||
return None
|
||||
|
||||
if security_analyzer_str.lower() == 'llm':
|
||||
return LLMSecurityAnalyzer()
|
||||
|
||||
# For unknown values, log a warning and return None
|
||||
_logger.warning(
|
||||
f'Unknown security analyzer value: {security_analyzer_str}. '
|
||||
'Supported values: "llm", "none". Defaulting to None.'
|
||||
)
|
||||
return None
|
||||
|
||||
def _select_confirmation_policy(
|
||||
self, confirmation_mode: bool, security_analyzer: str | None
|
||||
) -> ConfirmationPolicyBase:
|
||||
"""Choose confirmation policy using only mode flag and analyzer string."""
|
||||
if not confirmation_mode:
|
||||
return NeverConfirm()
|
||||
|
||||
analyzer_kind = (security_analyzer or '').lower()
|
||||
if analyzer_kind == 'llm':
|
||||
return ConfirmRisky()
|
||||
|
||||
return AlwaysConfirm()
|
||||
|
||||
async def _set_security_analyzer_from_settings(
|
||||
self,
|
||||
agent_server_url: str,
|
||||
session_api_key: str | None,
|
||||
conversation_id: UUID,
|
||||
security_analyzer_str: str | None,
|
||||
httpx_client: 'httpx.AsyncClient',
|
||||
) -> None:
|
||||
"""Set security analyzer on conversation using only the analyzer string.
|
||||
|
||||
Args:
|
||||
agent_server_url: URL of the agent server
|
||||
session_api_key: Session API key for authentication
|
||||
conversation_id: ID of the conversation to update
|
||||
security_analyzer_str: String value from settings
|
||||
httpx_client: HTTP client for making API requests
|
||||
"""
|
||||
|
||||
if session_api_key is None:
|
||||
return
|
||||
|
||||
security_analyzer = self._create_security_analyzer_from_string(
|
||||
security_analyzer_str
|
||||
)
|
||||
|
||||
# Only make API call if we have a security analyzer to set
|
||||
# (None is the default, so we can skip the call if it's None)
|
||||
if security_analyzer is None:
|
||||
return
|
||||
|
||||
try:
|
||||
# Prepare the request payload
|
||||
payload = {'security_analyzer': security_analyzer.model_dump()}
|
||||
|
||||
# Call agent server API to set security analyzer
|
||||
response = await httpx_client.post(
|
||||
f'{agent_server_url}/api/conversations/{conversation_id}/security_analyzer',
|
||||
json=payload,
|
||||
headers={'X-Session-API-Key': session_api_key},
|
||||
timeout=30.0,
|
||||
)
|
||||
response.raise_for_status()
|
||||
_logger.info(
|
||||
f'Successfully set security analyzer for conversation {conversation_id}'
|
||||
)
|
||||
except Exception as e:
|
||||
# Log error but don't fail conversation creation
|
||||
_logger.warning(
|
||||
f'Failed to set security analyzer for conversation {conversation_id}: {e}',
|
||||
exc_info=True,
|
||||
)
|
||||
|
||||
@@ -13,7 +13,6 @@ from pydantic import Field, SecretStr, TypeAdapter
|
||||
|
||||
from openhands.agent_server.models import (
|
||||
ConversationInfo,
|
||||
NeverConfirm,
|
||||
SendMessageRequest,
|
||||
StartConversationRequest,
|
||||
)
|
||||
@@ -72,7 +71,6 @@ from openhands.integrations.provider import ProviderType
|
||||
from openhands.sdk import Agent, AgentContext, LocalWorkspace
|
||||
from openhands.sdk.llm import LLM
|
||||
from openhands.sdk.secret import LookupSecret, StaticSecret
|
||||
from openhands.sdk.security.confirmation_policy import AlwaysConfirm
|
||||
from openhands.sdk.workspace.remote.async_remote_workspace import AsyncRemoteWorkspace
|
||||
from openhands.server.types import AppMode
|
||||
from openhands.tools.preset.default import (
|
||||
@@ -308,6 +306,16 @@ class LiveStatusAppConversationService(AppConversationServiceBase):
|
||||
)
|
||||
)
|
||||
|
||||
# Set security analyzer from settings
|
||||
user = await self.user_context.get_user_info()
|
||||
await self._set_security_analyzer_from_settings(
|
||||
agent_server_url,
|
||||
sandbox.session_api_key,
|
||||
info.id,
|
||||
user.security_analyzer,
|
||||
self.httpx_client,
|
||||
)
|
||||
|
||||
# Update the start task
|
||||
task.status = AppConversationStartTaskStatus.READY
|
||||
task.app_conversation_id = info.id
|
||||
@@ -749,8 +757,8 @@ class LiveStatusAppConversationService(AppConversationServiceBase):
|
||||
conversation_id=conversation_id,
|
||||
agent=agent,
|
||||
workspace=workspace,
|
||||
confirmation_policy=(
|
||||
AlwaysConfirm() if user.confirmation_mode else NeverConfirm()
|
||||
confirmation_policy=self._select_confirmation_policy(
|
||||
bool(user.confirmation_mode), user.security_analyzer
|
||||
),
|
||||
initial_message=initial_message,
|
||||
secrets=secrets,
|
||||
|
||||
@@ -1,11 +1,13 @@
|
||||
"""Unit tests for git functionality in AppConversationServiceBase.
|
||||
"""Unit tests for git and security functionality in AppConversationServiceBase.
|
||||
|
||||
This module tests the git-related functionality, specifically the clone_or_init_git_repo method
|
||||
and the recent bug fixes for git checkout operations.
|
||||
"""
|
||||
|
||||
import subprocess
|
||||
from types import MethodType
|
||||
from unittest.mock import AsyncMock, MagicMock, Mock, patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
|
||||
@@ -434,13 +436,298 @@ def test_create_condenser_plan_agent_with_custom_max_size(mock_condenser_class):
|
||||
mock_llm.model_copy.assert_called_once()
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Tests for security analyzer helpers
|
||||
# =============================================================================
|
||||
|
||||
|
||||
@pytest.mark.parametrize('value', [None, '', 'none', 'NoNe'])
|
||||
def test_create_security_analyzer_returns_none_for_empty_values(value):
|
||||
"""_create_security_analyzer_from_string returns None for empty/none values."""
|
||||
# Arrange
|
||||
service, _ = _create_service_with_mock_user_context(
|
||||
MockUserInfo(), bind_methods=('_create_security_analyzer_from_string',)
|
||||
)
|
||||
|
||||
# Act
|
||||
result = service._create_security_analyzer_from_string(value)
|
||||
|
||||
# Assert
|
||||
assert result is None
|
||||
|
||||
|
||||
def test_create_security_analyzer_returns_llm_analyzer():
|
||||
"""_create_security_analyzer_from_string returns LLMSecurityAnalyzer for llm string."""
|
||||
# Arrange
|
||||
security_analyzer_str = 'llm'
|
||||
service, _ = _create_service_with_mock_user_context(
|
||||
MockUserInfo(), bind_methods=('_create_security_analyzer_from_string',)
|
||||
)
|
||||
|
||||
# Act
|
||||
result = service._create_security_analyzer_from_string(security_analyzer_str)
|
||||
|
||||
# Assert
|
||||
from openhands.sdk.security.llm_analyzer import LLMSecurityAnalyzer
|
||||
|
||||
assert isinstance(result, LLMSecurityAnalyzer)
|
||||
|
||||
|
||||
def test_create_security_analyzer_logs_warning_for_unknown_value():
|
||||
"""_create_security_analyzer_from_string logs warning and returns None for unknown."""
|
||||
# Arrange
|
||||
unknown_value = 'custom'
|
||||
service, _ = _create_service_with_mock_user_context(
|
||||
MockUserInfo(), bind_methods=('_create_security_analyzer_from_string',)
|
||||
)
|
||||
|
||||
# Act
|
||||
with patch(
|
||||
'openhands.app_server.app_conversation.app_conversation_service_base._logger'
|
||||
) as mock_logger:
|
||||
result = service._create_security_analyzer_from_string(unknown_value)
|
||||
|
||||
# Assert
|
||||
assert result is None
|
||||
mock_logger.warning.assert_called_once()
|
||||
|
||||
|
||||
def test_select_confirmation_policy_when_disabled_returns_never_confirm():
|
||||
"""_select_confirmation_policy returns NeverConfirm when confirmation_mode is False."""
|
||||
# Arrange
|
||||
confirmation_mode = False
|
||||
security_analyzer = 'llm'
|
||||
service, _ = _create_service_with_mock_user_context(
|
||||
MockUserInfo(), bind_methods=('_select_confirmation_policy',)
|
||||
)
|
||||
|
||||
# Act
|
||||
policy = service._select_confirmation_policy(confirmation_mode, security_analyzer)
|
||||
|
||||
# Assert
|
||||
from openhands.sdk.security.confirmation_policy import NeverConfirm
|
||||
|
||||
assert isinstance(policy, NeverConfirm)
|
||||
|
||||
|
||||
def test_select_confirmation_policy_llm_returns_confirm_risky():
|
||||
"""_select_confirmation_policy uses ConfirmRisky when analyzer is llm."""
|
||||
# Arrange
|
||||
confirmation_mode = True
|
||||
security_analyzer = 'llm'
|
||||
service, _ = _create_service_with_mock_user_context(
|
||||
MockUserInfo(), bind_methods=('_select_confirmation_policy',)
|
||||
)
|
||||
|
||||
# Act
|
||||
policy = service._select_confirmation_policy(confirmation_mode, security_analyzer)
|
||||
|
||||
# Assert
|
||||
from openhands.sdk.security.confirmation_policy import ConfirmRisky
|
||||
|
||||
assert isinstance(policy, ConfirmRisky)
|
||||
|
||||
|
||||
@pytest.mark.parametrize('security_analyzer', [None, '', 'none', 'custom'])
|
||||
def test_select_confirmation_policy_non_llm_returns_always_confirm(
|
||||
security_analyzer,
|
||||
):
|
||||
"""_select_confirmation_policy falls back to AlwaysConfirm for non-llm values."""
|
||||
# Arrange
|
||||
confirmation_mode = True
|
||||
service, _ = _create_service_with_mock_user_context(
|
||||
MockUserInfo(), bind_methods=('_select_confirmation_policy',)
|
||||
)
|
||||
|
||||
# Act
|
||||
policy = service._select_confirmation_policy(confirmation_mode, security_analyzer)
|
||||
|
||||
# Assert
|
||||
from openhands.sdk.security.confirmation_policy import AlwaysConfirm
|
||||
|
||||
assert isinstance(policy, AlwaysConfirm)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_set_security_analyzer_skips_when_no_session_key():
|
||||
"""_set_security_analyzer_from_settings exits early without session_api_key."""
|
||||
# Arrange
|
||||
agent_server_url = 'https://agent.example.com'
|
||||
conversation_id = uuid4()
|
||||
httpx_client = AsyncMock()
|
||||
service, _ = _create_service_with_mock_user_context(
|
||||
MockUserInfo(),
|
||||
bind_methods=(
|
||||
'_create_security_analyzer_from_string',
|
||||
'_set_security_analyzer_from_settings',
|
||||
),
|
||||
)
|
||||
|
||||
with patch.object(service, '_create_security_analyzer_from_string') as mock_create:
|
||||
# Act
|
||||
await service._set_security_analyzer_from_settings(
|
||||
agent_server_url=agent_server_url,
|
||||
session_api_key=None,
|
||||
conversation_id=conversation_id,
|
||||
security_analyzer_str='llm',
|
||||
httpx_client=httpx_client,
|
||||
)
|
||||
|
||||
# Assert
|
||||
mock_create.assert_not_called()
|
||||
httpx_client.post.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_set_security_analyzer_skips_when_analyzer_none():
|
||||
"""_set_security_analyzer_from_settings skips API call when analyzer resolves to None."""
|
||||
# Arrange
|
||||
agent_server_url = 'https://agent.example.com'
|
||||
session_api_key = 'session-key'
|
||||
conversation_id = uuid4()
|
||||
httpx_client = AsyncMock()
|
||||
service, _ = _create_service_with_mock_user_context(
|
||||
MockUserInfo(),
|
||||
bind_methods=(
|
||||
'_create_security_analyzer_from_string',
|
||||
'_set_security_analyzer_from_settings',
|
||||
),
|
||||
)
|
||||
|
||||
with patch.object(
|
||||
service, '_create_security_analyzer_from_string', return_value=None
|
||||
) as mock_create:
|
||||
# Act
|
||||
await service._set_security_analyzer_from_settings(
|
||||
agent_server_url=agent_server_url,
|
||||
session_api_key=session_api_key,
|
||||
conversation_id=conversation_id,
|
||||
security_analyzer_str='none',
|
||||
httpx_client=httpx_client,
|
||||
)
|
||||
|
||||
# Assert
|
||||
mock_create.assert_called_once_with('none')
|
||||
httpx_client.post.assert_not_called()
|
||||
|
||||
|
||||
class DummyAnalyzer:
|
||||
"""Simple analyzer stub for testing model_dump contract."""
|
||||
|
||||
def __init__(self, payload: dict):
|
||||
self._payload = payload
|
||||
|
||||
def model_dump(self) -> dict:
|
||||
return self._payload
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_set_security_analyzer_successfully_calls_agent_server():
|
||||
"""_set_security_analyzer_from_settings posts analyzer payload when available."""
|
||||
# Arrange
|
||||
agent_server_url = 'https://agent.example.com'
|
||||
session_api_key = 'session-key'
|
||||
conversation_id = uuid4()
|
||||
analyzer_payload = {'type': 'llm'}
|
||||
httpx_client = AsyncMock()
|
||||
http_response = MagicMock()
|
||||
http_response.raise_for_status = MagicMock()
|
||||
httpx_client.post.return_value = http_response
|
||||
service, _ = _create_service_with_mock_user_context(
|
||||
MockUserInfo(),
|
||||
bind_methods=(
|
||||
'_create_security_analyzer_from_string',
|
||||
'_set_security_analyzer_from_settings',
|
||||
),
|
||||
)
|
||||
|
||||
analyzer = DummyAnalyzer(analyzer_payload)
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
service,
|
||||
'_create_security_analyzer_from_string',
|
||||
return_value=analyzer,
|
||||
) as mock_create,
|
||||
patch(
|
||||
'openhands.app_server.app_conversation.app_conversation_service_base._logger'
|
||||
) as mock_logger,
|
||||
):
|
||||
# Act
|
||||
await service._set_security_analyzer_from_settings(
|
||||
agent_server_url=agent_server_url,
|
||||
session_api_key=session_api_key,
|
||||
conversation_id=conversation_id,
|
||||
security_analyzer_str='llm',
|
||||
httpx_client=httpx_client,
|
||||
)
|
||||
|
||||
# Assert
|
||||
mock_create.assert_called_once_with('llm')
|
||||
httpx_client.post.assert_awaited_once_with(
|
||||
f'{agent_server_url}/api/conversations/{conversation_id}/security_analyzer',
|
||||
json={'security_analyzer': analyzer_payload},
|
||||
headers={'X-Session-API-Key': session_api_key},
|
||||
timeout=30.0,
|
||||
)
|
||||
http_response.raise_for_status.assert_called_once()
|
||||
mock_logger.info.assert_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_set_security_analyzer_logs_warning_on_failure():
|
||||
"""_set_security_analyzer_from_settings warns but does not raise on errors."""
|
||||
# Arrange
|
||||
agent_server_url = 'https://agent.example.com'
|
||||
session_api_key = 'session-key'
|
||||
conversation_id = uuid4()
|
||||
analyzer_payload = {'type': 'llm'}
|
||||
httpx_client = AsyncMock()
|
||||
httpx_client.post.side_effect = RuntimeError('network down')
|
||||
service, _ = _create_service_with_mock_user_context(
|
||||
MockUserInfo(),
|
||||
bind_methods=(
|
||||
'_create_security_analyzer_from_string',
|
||||
'_set_security_analyzer_from_settings',
|
||||
),
|
||||
)
|
||||
|
||||
analyzer = DummyAnalyzer(analyzer_payload)
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
service,
|
||||
'_create_security_analyzer_from_string',
|
||||
return_value=analyzer,
|
||||
) as mock_create,
|
||||
patch(
|
||||
'openhands.app_server.app_conversation.app_conversation_service_base._logger'
|
||||
) as mock_logger,
|
||||
):
|
||||
# Act
|
||||
await service._set_security_analyzer_from_settings(
|
||||
agent_server_url=agent_server_url,
|
||||
session_api_key=session_api_key,
|
||||
conversation_id=conversation_id,
|
||||
security_analyzer_str='llm',
|
||||
httpx_client=httpx_client,
|
||||
)
|
||||
|
||||
# Assert
|
||||
mock_create.assert_called_once_with('llm')
|
||||
httpx_client.post.assert_awaited_once()
|
||||
mock_logger.warning.assert_called()
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Tests for _configure_git_user_settings
|
||||
# =============================================================================
|
||||
|
||||
|
||||
def _create_service_with_mock_user_context(user_info: MockUserInfo) -> tuple:
|
||||
"""Create a mock service with the actual _configure_git_user_settings method.
|
||||
def _create_service_with_mock_user_context(
|
||||
user_info: MockUserInfo, bind_methods: tuple[str, ...] | None = None
|
||||
) -> tuple:
|
||||
"""Create a mock service with selected real methods bound for testing.
|
||||
|
||||
Uses MagicMock for the service but binds the real method for testing.
|
||||
|
||||
@@ -452,13 +739,16 @@ def _create_service_with_mock_user_context(user_info: MockUserInfo) -> tuple:
|
||||
# Create a simple mock service and set required attribute
|
||||
service = MagicMock()
|
||||
service.user_context = mock_user_context
|
||||
methods_to_bind = ['_configure_git_user_settings']
|
||||
if bind_methods:
|
||||
methods_to_bind.extend(bind_methods)
|
||||
# Remove potential duplicates while keeping order
|
||||
methods_to_bind = list(dict.fromkeys(methods_to_bind))
|
||||
|
||||
# Bind the actual method from the real class to test real implementation
|
||||
service._configure_git_user_settings = (
|
||||
lambda workspace: AppConversationServiceBase._configure_git_user_settings(
|
||||
service, workspace
|
||||
)
|
||||
)
|
||||
# Bind actual methods from the real class to test implementations directly
|
||||
for method_name in methods_to_bind:
|
||||
real_method = getattr(AppConversationServiceBase, method_name)
|
||||
setattr(service, method_name, MethodType(real_method, service))
|
||||
|
||||
return service, mock_user_context
|
||||
|
||||
|
||||
@@ -153,6 +153,7 @@ class TestExperimentManagerIntegration:
|
||||
llm_api_key=None,
|
||||
confirmation_mode=False,
|
||||
condenser_max_size=None,
|
||||
security_analyzer=None,
|
||||
)
|
||||
|
||||
async def get_secrets(self):
|
||||
|
||||
Reference in New Issue
Block a user